Bug 125139
| Summary: | [ata] bugs in ATAPI CD tray control | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Jaakko Heinonen <jh> | ||||
| Component: | kern | Assignee: | freebsd-bugs (Nobody) <bugs> | ||||
| Status: | Open --- | ||||||
| Severity: | Affects Only Me | CC: | Alexander88207, emaste | ||||
| Priority: | Normal | ||||||
| Version: | 7.0-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
Jaakko Heinonen
2008-07-01 11:00:11 UTC
Responsible Changed From-To: freebsd-bugs->philip I'll take it. State Changed From-To: open->analyzed I have verified this bug. The refcounting of opens appears correct, but seems to need locking. On 2008-10-17, brooks@FreeBSD.org wrote: > I have verified this bug. The refcounting of opens appears correct, > but seems to need locking. I assumed that the GEOM topology lock is held during acd_geom_ioctl() and acd_geom_access() calls but indeed it's not true for acd_geom_ioctl(). Looks like acd_geom_ioctl() misses locking completely. -- Jaakko On 2008-10-17, brooks@FreeBSD.org wrote: > I have verified this bug. The refcounting of opens appears correct, but > seems to need locking. Here is an updated patch which uses a per device sx lock for softc locking. There are many sleepable code paths which makes using a mutex hard. (The patch contains also an unrelated fix to check g_modevent() return value in acd_modevent(). Also I added a check for NULL cdp to acd_geom_access(). cdp could be NULL if the device is detaching.) %%% Index: sys/dev/ata/atapi-cd.c =================================================================== --- sys/dev/ata/atapi-cd.c (revision 185028) +++ sys/dev/ata/atapi-cd.c (working copy) @@ -124,6 +124,7 @@ acd_attach(device_t dev) device_printf(dev, "out of memory\n"); return ENOMEM; } + sx_init(&cdp->lock, "ATAPI CD softc lock"); cdp->block_size = 2048; device_set_ivars(dev, cdp); ATA_SETMODE(device_get_parent(dev), dev); @@ -140,7 +141,7 @@ static int acd_detach(device_t dev) { g_waitfor_event(acd_geom_detach, dev, M_WAITOK, NULL); - return 0; + return (device_get_ivars(dev) == NULL) ? 0 : EBUSY; } static void @@ -191,6 +192,13 @@ acd_geom_detach(void *arg, int flag) { struct acd_softc *cdp = device_get_ivars(arg); + g_topology_assert(); + sx_slock(&cdp->lock); + if (cdp->grefcnt != 0) { + sx_sunlock(&cdp->lock); + return; + } + /* signal geom so we dont get any further requests */ g_wither_geom(cdp->gp, ENXIO); @@ -199,6 +207,8 @@ acd_geom_detach(void *arg, int flag) /* dont leave anything behind */ device_set_ivars(arg, NULL); + sx_sunlock(&cdp->lock); + sx_destroy(&cdp->lock); free(cdp, M_ACD); } @@ -213,6 +223,8 @@ acd_geom_ioctl(struct g_provider *pp, u_ if (!cdp) return ENXIO; + sx_xlock(&cdp->lock); + if (atadev->flags & ATA_D_MEDIA_CHANGED) { switch (cmd) { case CDIOCRESET: @@ -246,11 +258,19 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCALLOW: + if (cdp->grefcnt != 1 || pp->acr != 1) { + error = EBUSY; + break; + } error = acd_prevent_allow(dev, 0); cdp->flags &= ~F_LOCKED; break; case CDIOCPREVENT: + if (cdp->grefcnt != 1 || pp->acr != 1) { + error = EBUSY; + break; + } error = acd_prevent_allow(dev, 1); cdp->flags |= F_LOCKED; break; @@ -266,7 +286,7 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCEJECT: - if (pp->acr != 1) { + if (cdp->grefcnt != 1 || pp->acr != 1) { error = EBUSY; break; } @@ -274,8 +294,10 @@ acd_geom_ioctl(struct g_provider *pp, u_ break; case CDIOCCLOSE: - if (pp->acr != 1) + if (cdp->grefcnt != 1 || pp->acr != 1) { + error = EBUSY; break; + } error = acd_tray(dev, 1); break; @@ -678,6 +700,9 @@ acd_geom_ioctl(struct g_provider *pp, u_ default: error = ata_device_ioctl(dev, cmd, addr); } + + sx_xunlock(&cdp->lock); + return error; } @@ -691,6 +716,9 @@ acd_geom_access(struct g_provider *pp, i 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; int timeout = 60, track; + if (!cdp) + return ENXIO; + if (!(request = ata_alloc_request())) return ENOMEM; @@ -712,13 +740,19 @@ acd_geom_access(struct g_provider *pp, i } ata_free_request(request); + sx_xlock(&cdp->lock); + if (pp->acr == 0) { + cdp->grefcnt++; acd_prevent_allow(dev, 1); cdp->flags |= F_LOCKED; acd_read_toc(dev); } - if (dr + pp->acr == 0) { + if (dr + pp->acr == 0) + cdp->grefcnt--; + + if (cdp->grefcnt == 0) { acd_prevent_allow(dev, 0); cdp->flags &= ~F_LOCKED; } @@ -734,6 +768,8 @@ acd_geom_access(struct g_provider *pp, i } pp->mediasize *= pp->sectorsize; + sx_xunlock(&cdp->lock); + return 0; } @@ -748,7 +784,10 @@ acd_geom_start(struct bio *bp) return; } + sx_slock(&cdp->lock); + if (bp->bio_cmd == BIO_READ && cdp->disk_size == -1) { + sx_sunlock(&cdp->lock); g_io_deliver(bp, EIO); return; } @@ -782,6 +821,7 @@ acd_geom_start(struct bio *bp) bp2 = bp3; } } + sx_sunlock(&cdp->lock); } static void @@ -1906,8 +1946,7 @@ static devclass_t acd_devclass; static int acd_modevent(module_t mod, int what, void *arg) { - g_modevent(0, what, &acd_class); - return 0; + return g_modevent(0, what, &acd_class); } DRIVER_MODULE(acd, ata, acd_driver, acd_devclass, acd_modevent, NULL); Index: sys/dev/ata/atapi-cd.h =================================================================== --- sys/dev/ata/atapi-cd.h (revision 185028) +++ sys/dev/ata/atapi-cd.h (working copy) @@ -312,4 +312,7 @@ struct acd_softc { u_int32_t iomax; /* Max I/O request (bytes) */ struct g_geom *gp; /* geom instance */ struct g_provider *pp[MAXTRK+1]; /* providers */ + u_int grefcnt; /* number of open geom + providers */ + struct sx lock; }; %%% Responsible Changed From-To: philip->brooks Steal this one from philip. I don't have the hardware to test this patch with me, but I'll do it once I get home. Return to pool. For bugs matching the following conditions: - Status == In Progress - Assignee == "bugs@FreeBSD.org" - Last Modified Year <= 2017 Do - Set Status to "Open" Keyword:
patch
or patch-ready
– in lieu of summary line prefix:
[patch]
* bulk change for the keyword
* summary lines may be edited manually (not in bulk).
Keyword descriptions and search interface:
<https://bugs.freebsd.org/bugzilla/describekeywords.cgi>
|
