Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Wed, Jun 13, 2012 at 05:15:08PM -0400, Alan Stern wrote:
> On Wed, 13 Jun 2012, Greg KH wrote:
> 
> > > +	struct device_attribute	port_control_attr;
> > > +	struct device_attribute	port_state_attr;
> > > +	enum port_power_policy port_power_policy;
> > 
> > Why do you need an attribute per port here?  Shouldn't they just be
> > static variables?  Why duplicate them for every port?
> 
> If they were static, there would be no way for the store and show
> methods to know which port they were called for.  That's because the
> ports aren't separate kobjects; all these port attributes are bound to
> the hub device.

Ports should be a struct device if we are going to hang attributes off
of them, otherwise userspace can get very confused.

> > > +static ssize_t show_port_power_state(struct device *dev,
> > > +	struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct usb_device *udev = to_usb_device(dev->parent);
> > > +	struct usb_hub_port *hub_port = container_of(attr,
> > > +		struct usb_hub_port, port_state_attr);
> 
> You can see it here.  The only way to get the port is by seeing where
> the attribute is stored.

Ick, that's nasty.

How about making a port a real device?  That should solve this problem,
right?  And I thought I recommended doing that a month or so ago, what
happened with that proposal?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html