Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs

["Love, Robert W" <robert.w.love@intel.com>]

On 03/16/2012 05:25 PM, Greg KH wrote:
> On Fri, Mar 16, 2012 at 12:36:56PM -0700, Robert Love wrote:

<snip>

> diff --git a/Documentation/ABI/testing/sysfs-class-fcoe b/Documentation/ABI/testing/sysfs-class-fcoe
> new file mode 100644
> index 0000000..e9cd7e9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fcoe
> @@ -0,0 +1,77 @@
> +What:		/sys/class/fcoe_ctlr/ctlr_X
> We are really trying to stay away from new classes being created.  Why
> can't this be a bus and have the devices attach to that instead?  You
> can have one bus with both types of "devices" attached to it, making
> things a bit simpler in the end.

<snip>

> +	dev_set_name(&ctlr->dev, "ctlr_%d", ctlr->id);
> +	error = device_add(&ctlr->dev);
> +	if (error)
> +		goto out_del_q2;
> +
> +	error = sysfs_create_group(&ctlr->dev.kobj,
> +				&fcoe_ctlr_attribute_group);
> +	if (error)
> +		goto out_del_dev;
> +
> +	error = sysfs_create_group(&ctlr->dev.kobj,
> +				&fcoe_ctlr_lesb_attribute_group);
> +	if (error)
> +		goto out_del_dev;
> You just raced userspace by creating your attributes after device_add()
> causing lots of problems in the longrun.  Why not make these default
> attribute groups that the driver core automatically creates for you
> properly?  That also makes your error path simpler, as well as your
> cleanup path for when this device goes away.
>

Hi Greg. I have a local series that addresses most of the comments 
you've made. I have a question about the above two requests.

I've converted my series to create a 'fcoe bus' and now instances of the 
'fcoe_ctlr' and 'fcoe_fcf' are on the 'fcoe bus.' I did not create any 
drivers for the bus; I'm simply adding devices with their device pointer 
set to my 'fcoe bus' (dev->bus = &fcoe_bus_type) so that these devices 
are grouped under /sys/bus/fcoe/. This change results in the following, 
which I think is what you want.

[root@bubba ~]# ls -l /sys/bus/fcoe/drivers
total 0
[root@bubba ~]# ls -l /sys/bus/fcoe/devices/
total 0
lrwxrwxrwx 1 root root 0 Mar 19 18:20 ctlr_0 -> 
../../../devices/virtual/net/eth4.172-fcoe/ctlr_0
lrwxrwxrwx 1 root root 0 Mar 19 18:20 fcf_0 -> 
../../../devices/virtual/net/eth4.172-fcoe/ctlr_0/fcf_0

Is there a way for me to make default attribute groups for each of these 
device types, as you suggest, without having a 'fcoe_ctlr' driver and a 
'fcoe_fcf' driver, or does your suggestion imply that I should be 
creating drivers for these two types?

I think I'm limited to having default attributes for any device on the 
'fcoe bus,' which is not what I want because my fcoe_fcf and fcoe_ctlr 
devices do not share attributes. Or to create a drivers which have 
default attribute groups and therefore the core will create attributes 
as devices are matched with their drivers. Since I'm dealing with a 
'subsystem bus' and not a real bus, I'm not sure if it's appropriate for 
me to be creating virtual subsystem drivers...

Thanks, //Rob