Re: [PATCH 03/10] SCSI: fcoe: convert to use BUS_ATTR_WO

Re: [PATCH 03/10] SCSI: fcoe: convert to use BUS_ATTR_WO

[James Bottomley]

[scsi list cc added]
On Fri, 2018-12-21 at 08:54 +0100, Greg Kroah-Hartman wrote:
> We are trying to get rid of BUS_ATTR() and the usage of that in the
> fcoe driver can be trivially converted to use BUS_ATTR_WO(), so use
> that instead.
> 
> At the same time remove a unneeded EXPORT_SYMBOL() marking for the
> sysfs callback function we are renaming, no idea of how that got into
> the tree...

The EXPORT_SYMBOL removal is fine, but

[...]
> --- a/include/scsi/libfcoe.h
> +++ b/include/scsi/libfcoe.h
> @@ -405,10 +405,8 @@ int fcoe_transport_attach(struct fcoe_transport
> *ft);
>  int fcoe_transport_detach(struct fcoe_transport *ft);
> 
>  /* sysfs store handler for ctrl_control interface */
> -ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
> -			       const char *buf, size_t count);
> -ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
> -				const char *buf, size_t count);
> +ssize_t ctlr_create_store(struct bus_type *bus, const char *buf,
> size_t count);
> +ssize_t ctlr_destroy_store(struct bus_type *bus, const char *buf,
> size_t count);

You're really damaging our prefix namespace here.  It looks like the
ctlr_ name is a farly recent addition for sysfs (only myra/b) use it in
SCSI but it's inviting symbol clashes.

Since the XXX_ATTR_RO seem to be defined with only local file usage in
mind, I think we need static functions to shim the problem, like below
(provided this is the only instance, because if it isn't, you probably
need a XXX_ATTR_WO_prefix() macro)

James

---

diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index 5c8310bade61..88e5c26ce381 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -671,8 +671,20 @@ static const struct device_type fcoe_fcf_device_type = {
 	.release = fcoe_fcf_device_release,
 };
 
-static BUS_ATTR(ctlr_create, S_IWUSR, NULL, fcoe_ctlr_create_store);
-static BUS_ATTR(ctlr_destroy, S_IWUSR, NULL, fcoe_ctlr_destroy_store);
+static ssize_t ctlr_create_store(struct bus_type *bus, const char *buf,
+				 size_t count)
+{
+	return fcoe_ctlr_create_store(bus, buf, count);
+}
+
+static ssize_t ctlr_destroy_store(struct bus_type *bus, const char *buf,
+				  size_t count)
+{
+	return fcoe_ctlr_destroy_store(bus, buf, count);
+}
+
+static BUS_ATTR_WO(ctlr_create);
+static BUS_ATTR_WO(ctlr_destroy);
 
 static struct attribute *fcoe_bus_attrs[] = {
 	&bus_attr_ctlr_create.attr,

Re: [PATCH 03/10] SCSI: fcoe: convert to use BUS_ATTR_WO

[Hannes Reinecke]

On 12/21/18 4:29 PM, James Bottomley wrote:
> [scsi list cc added]
> On Fri, 2018-12-21 at 08:54 +0100, Greg Kroah-Hartman wrote:
>> We are trying to get rid of BUS_ATTR() and the usage of that in the
>> fcoe driver can be trivially converted to use BUS_ATTR_WO(), so use
>> that instead.
>>
>> At the same time remove a unneeded EXPORT_SYMBOL() marking for the
>> sysfs callback function we are renaming, no idea of how that got into
>> the tree...
> 
> The EXPORT_SYMBOL removal is fine, but
> 
> [...]
>> --- a/include/scsi/libfcoe.h
>> +++ b/include/scsi/libfcoe.h
>> @@ -405,10 +405,8 @@ int fcoe_transport_attach(struct fcoe_transport
>> *ft);
>>   int fcoe_transport_detach(struct fcoe_transport *ft);
>>
>>   /* sysfs store handler for ctrl_control interface */
>> -ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
>> -			       const char *buf, size_t count);
>> -ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
>> -				const char *buf, size_t count);
>> +ssize_t ctlr_create_store(struct bus_type *bus, const char *buf,
>> size_t count);
>> +ssize_t ctlr_destroy_store(struct bus_type *bus, const char *buf,
>> size_t count);
> 
> You're really damaging our prefix namespace here.  It looks like the
> ctlr_ name is a farly recent addition for sysfs (only myra/b) use it in
> SCSI but it's inviting symbol clashes.
> 
Hmm. I was under the impression that all sysfs functions from myrb/myrs 
are local, hence I would not need to prefix them.
If this isn't the case I definitely will be fixing them.

But in any case, if possible any sysfs function should be local to the 
driver; no-one else should ever attempt to use them.
And we should be making it so if that's not the case.

Cheers,

Hannes

Re: [PATCH 03/10] SCSI: fcoe: convert to use BUS_ATTR_WO

[Greg Kroah-Hartman]

On Fri, Dec 28, 2018 at 01:50:53PM +0100, Hannes Reinecke wrote:
> On 12/21/18 4:29 PM, James Bottomley wrote:
> > [scsi list cc added]
> > On Fri, 2018-12-21 at 08:54 +0100, Greg Kroah-Hartman wrote:
> > > We are trying to get rid of BUS_ATTR() and the usage of that in the
> > > fcoe driver can be trivially converted to use BUS_ATTR_WO(), so use
> > > that instead.
> > > 
> > > At the same time remove a unneeded EXPORT_SYMBOL() marking for the
> > > sysfs callback function we are renaming, no idea of how that got into
> > > the tree...
> > 
> > The EXPORT_SYMBOL removal is fine, but
> > 
> > [...]
> > > --- a/include/scsi/libfcoe.h
> > > +++ b/include/scsi/libfcoe.h
> > > @@ -405,10 +405,8 @@ int fcoe_transport_attach(struct fcoe_transport
> > > *ft);
> > >   int fcoe_transport_detach(struct fcoe_transport *ft);
> > > 
> > >   /* sysfs store handler for ctrl_control interface */
> > > -ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
> > > -			       const char *buf, size_t count);
> > > -ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
> > > -				const char *buf, size_t count);
> > > +ssize_t ctlr_create_store(struct bus_type *bus, const char *buf,
> > > size_t count);
> > > +ssize_t ctlr_destroy_store(struct bus_type *bus, const char *buf,
> > > size_t count);
> > 
> > You're really damaging our prefix namespace here.  It looks like the
> > ctlr_ name is a farly recent addition for sysfs (only myra/b) use it in
> > SCSI but it's inviting symbol clashes.
> > 
> Hmm. I was under the impression that all sysfs functions from myrb/myrs are
> local, hence I would not need to prefix them.
> If this isn't the case I definitely will be fixing them.
> 
> But in any case, if possible any sysfs function should be local to the
> driver; no-one else should ever attempt to use them.
> And we should be making it so if that's not the case.

This is all in the same "driver", just that the driver is spread out
over multiple files.

James, thanks for the fixup, I'll go respin this and break this up into
two patches and resend in a bit.

thanks,

greg k-h