Re: [Resend PATCH V3 1/4] usb: add struct usb_hub_port to store port related members.

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Wed, Jun 13, 2012 at 05:22:48PM -0400, Alan Stern wrote:
> On Wed, 13 Jun 2012, Greg KH wrote:
> 
> > On Wed, Jun 13, 2012 at 11:07:17AM +0800, Lan Tianyu wrote:
> > > Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
> > > struct usb_hub_port perspectively for each ports to store private data.
> > > 
> > > Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> > > ---
> > >  drivers/usb/core/hub.c |   29 ++++++++++++++++-------------
> > >  1 files changed, 16 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 04fb834..512f0d5 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -37,6 +37,10 @@
> > >  #endif
> > >  #endif
> > >  
> > > +struct usb_hub_port {
> > > +	void			*port_owner;
> > 
> > What?  Why a void pointer?  You should almost _never_ use a void pointer
> > in kernel code, it's bad form, and it means you are trying to hide
> > something here, when it shouldn't be hidden, as we should know the real
> > type, right?
> 
> That's my fault; Lan is simply moving code that I wrote.
> 
> The void * values are simply cookies as far as the hub driver is
> concerned -- the meaning is _intended_ to be hidden and the pointers
> are never dereferenced.  All that matters about them is whether two
> values are the same or different.  The actual pointers stored in
> port_owner are created by usbfs; they are pointers to the internal data
> structures defined in devio.c.
> 
> If you really want, we could change void * to struct dev_state *.  I'm 
> not sure this would make things any better or clearer.

Yes, it would make it clearer when we read the code, seing dev_state *,
makes me know that we know what is going on.  A void * makes me know we
have no clue as to what is going on :)

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