Messages in this thread | | | Subject | Re: [patch 13/13] s390: dasd device identifiers. | From | Martin Schwidefsky <> | Date | Tue, 25 Apr 2006 10:28:13 +0200 |
| |
On Mon, 2006-04-24 at 16:49 -0700, Andrew Morton wrote: > Martin Schwidefsky <schwidefsky@de.ibm.com> wrote: > > > > @@ -45,6 +45,7 @@ struct dasd_devmap { > > unsigned int devindex; > > unsigned short features; > > struct dasd_device *device; > > + struct dasd_uid uid; > > }; > > Someone's missing a TAB key.
I have to find some script that checks for the damned tabs.
> > +static ssize_t > > +dasd_alias_show(struct device *dev, struct device_attribute *attr, char *buf) > > +{ > > + struct dasd_devmap *devmap; > > + int alias; > > + > > + devmap = dasd_find_busid(dev->bus_id); > > + spin_lock(&dasd_devmap_lock); > > + if (!IS_ERR(devmap)) > > + alias = devmap->uid.alias; > > + else > > + alias = 0; > > + spin_unlock(&dasd_devmap_lock); > > + > > + return sprintf(buf, alias ? "1\n" : "0\n"); > > +} > > The locking is suspicious. We take a spinlock just for a single read?
We could remove the locking for single read/writes, devmap structures are never deleted so it is safe to access a single value after the dasd_find_busid. But it is not so easy for the vendor and serial string, these are not single values so we need some kind of locking. To keep things symmetrical the lock is used for alias as well. There is a bug though, only the pointer to the vendor/serial string is read under the lock, the copy is done outside the lock. If that races with a dasd_set_uid call funny things might happen. The copy needs to be done inside the lock. Horst ?
-- blue skies, Martin.
Martin Schwidefsky Linux for zSeries Development & Services IBM Deutschland Entwicklung GmbH
"Reality continues to ruin my life." - Calvin.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |