From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups To: Christoph Hellwig , Hannes Reinecke Cc: Jens Axboe , Sagi Grimberg , Keith Busch , Bart van Assche , linux-block@vger.kernel.org, linux-nvme@lists.infradead.org References: <20180905070053.26239-1-hare@suse.de> <20180905070053.26239-3-hare@suse.de> <20180905131821.GA26956@lst.de> <20180905134507.GA27448@lst.de> From: Hannes Reinecke Message-ID: <5567c5dd-167c-baa5-c85a-90592e2db84e@suse.com> Date: Thu, 6 Sep 2018 11:56:59 +0200 MIME-Version: 1.0 In-Reply-To: <20180905134507.GA27448@lst.de> Content-Type: text/plain; charset=utf-8 List-ID: On 09/05/2018 03:45 PM, Christoph Hellwig wrote: > On Wed, Sep 05, 2018 at 03:32:03PM +0200, Hannes Reinecke wrote: >> On 09/05/2018 03:18 PM, Christoph Hellwig wrote: >>> On Wed, Sep 05, 2018 at 09:00:50AM +0200, Hannes Reinecke wrote: >>>> We should be registering the ns_id attribute as default sysfs >>>> attribute groups, otherwise we have a race condition between >>>> the uevent and the attributes appearing in sysfs. >>> >>> Please give Bart credit for his work, as the lightnvm bits are almost >>> bigger than the rest. >>> >> Okay, will be doing so. >> >>>> +static umode_t nvm_dev_attrs_visible(struct kobject *kobj, >>>> + struct attribute *attr, int index) >>>> { >>>> + struct device *dev = container_of(kobj, struct device, kobj); >>>> + struct gendisk *disk = dev_to_disk(dev); >>>> + struct nvme_ns *ns = disk->private_data; >>>> struct nvm_dev *ndev = ns->ndev; >>>> + struct device_attribute *dev_attr = >>>> + container_of(attr, typeof(*dev_attr), attr); >>>> >>>> + if (dev_attr->show == nvm_dev_attr_show) >>>> + return attr->mode; >>>> >>>> + switch (ndev ? ndev->geo.major_ver_id : 0) { >>> >>> How could ndev be zero here? >>> >> For 'normal' NVMe devices (ie non-lightnvm). As we now register all >> sysfs attributes (including the lightnvm ones) per default we'll need to >> blank them out for non-lightnvm devices. > > But then we need to exit early at the beginning of the function, > as we should not register attributes using nvm_dev_attr_show (or > anything else for that matter) either. > Okay, will be fixing it up. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.com +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) From mboxrd@z Thu Jan 1 00:00:00 1970 From: hare@suse.com (Hannes Reinecke) Date: Thu, 6 Sep 2018 11:56:59 +0200 Subject: [PATCH 2/5] nvme: register ns_id attributes as default sysfs groups In-Reply-To: <20180905134507.GA27448@lst.de> References: <20180905070053.26239-1-hare@suse.de> <20180905070053.26239-3-hare@suse.de> <20180905131821.GA26956@lst.de> <20180905134507.GA27448@lst.de> Message-ID: <5567c5dd-167c-baa5-c85a-90592e2db84e@suse.com> On 09/05/2018 03:45 PM, Christoph Hellwig wrote: > On Wed, Sep 05, 2018@03:32:03PM +0200, Hannes Reinecke wrote: >> On 09/05/2018 03:18 PM, Christoph Hellwig wrote: >>> On Wed, Sep 05, 2018@09:00:50AM +0200, Hannes Reinecke wrote: >>>> We should be registering the ns_id attribute as default sysfs >>>> attribute groups, otherwise we have a race condition between >>>> the uevent and the attributes appearing in sysfs. >>> >>> Please give Bart credit for his work, as the lightnvm bits are almost >>> bigger than the rest. >>> >> Okay, will be doing so. >> >>>> +static umode_t nvm_dev_attrs_visible(struct kobject *kobj, >>>> + struct attribute *attr, int index) >>>> { >>>> + struct device *dev = container_of(kobj, struct device, kobj); >>>> + struct gendisk *disk = dev_to_disk(dev); >>>> + struct nvme_ns *ns = disk->private_data; >>>> struct nvm_dev *ndev = ns->ndev; >>>> + struct device_attribute *dev_attr = >>>> + container_of(attr, typeof(*dev_attr), attr); >>>> >>>> + if (dev_attr->show == nvm_dev_attr_show) >>>> + return attr->mode; >>>> >>>> + switch (ndev ? ndev->geo.major_ver_id : 0) { >>> >>> How could ndev be zero here? >>> >> For 'normal' NVMe devices (ie non-lightnvm). As we now register all >> sysfs attributes (including the lightnvm ones) per default we'll need to >> blank them out for non-lightnvm devices. > > But then we need to exit early at the beginning of the function, > as we should not register attributes using nvm_dev_attr_show (or > anything else for that matter) either. > Okay, will be fixing it up. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare at suse.com +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: F. Imend?rffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG N?rnberg)