Re: Staging: vt6656 ?

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Thursday 02 July 2009 20:22:08 Forest Bond wrote:
> Hi,
> 
> On Thu, Jul 02, 2009 at 07:45:49PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 28 June 2009 18:47:16 Forest Bond wrote:
> > > On Sun, Jun 28, 2009 at 05:59:45PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > [ I'll later setup vt665x branch of my misc.git tree, merge your patches,
> > > >   merge all outstanding vt6655 patches from Alexander and investigate a bit
> > > >   more whether merge of vt665x drivers is feasible and what needs to be
> > > >   done if so.. ]
> > > 
> > > Good.
> > 
> > The temporary tree is here:
> > 
> > git://git.kernel.org:/pub/scm/linux/kernel/git/bart/misc.git vt665x
> > 
> > and I'll happily apply patches to it till Greg digs out from under the
> > overdue patch queues.. :)
> 
> Thanks for doing this, Bartlomiej.
> 
> > > FYI, there is a known issue with the drivers as I've submitted them that causes
> > > lock-ups.  Please see the attached message for a suggested fix.
> > 
> > I think that all netdev_priv() changes should be reverted for now:
> 
> I'm happy to defer to you on this.  I don't really understand the code, to be
> frank.  However, if those changes are simply reverted, the driver will not
> compile.  I assume that you mean those areas should be removed?

Uh, you're right.. we need to use netdev_priv().

> > --- a/drivers/staging/vt6655/wpactl.c
> > +++ b/drivers/staging/vt6655/wpactl.c
> > @@ -112,14 +112,17 @@ static void wpadev_setup(struct net_device *dev)
> >  
> >  static int wpa_init_wpadev(PSDevice pDevice)
> >  {
> > +    PSDevice wpadev_priv;
> >  	struct net_device *dev = pDevice->dev;
> >           int ret=0;
> >  
> > -	pDevice->wpadev = alloc_netdev(0, "vntwpa", wpadev_setup);
> > +	pDevice->wpadev = alloc_netdev(sizeof(PSDevice), "vntwpa", wpadev_setup);
> >  	if (pDevice->wpadev == NULL)
> >  		return -ENOMEM;
> >  
> > -	pDevice->wpadev->priv = pDevice;
> > +    wpadev_priv = netdev_priv(pDevice->wpadev);
> > +    *wpadev_priv = *pDevice;
> > +
> >  	memcpy(pDevice->wpadev->dev_addr, dev->dev_addr, U_ETHER_ADDR_LEN);
> >           pDevice->wpadev->base_addr = dev->base_addr;
> >  	pDevice->wpadev->irq = dev->irq;
> > 
> > This will copy the current state of pDevice to newly allocated private part
> > of ->apdev but later modifications to the original pDevice won't be seen if
> > we access it through netdev_priv(pDevice->apdev) instead of apdev->priv.

[ it should be wpadev not apdev in the above description ]

> > [ I don't know whether this is a problem currently but it looks suspicious. ]
> 
> Agreed.  I gave this a best effort, but was not very confident about the result.

The code is puzzling (at best) -- I still don't know why do we need separate
netdev structure for hostap or wpactl functionality..

> Feel free to aggressively rework my changes if it seems appropriate.

I don't think there is a need for it and I would like to avoid that
since I'm also involved in other drivers/staging/ drivers (besides you
and Alexander are doing just fine :).

Seems like all we need to do to fix the problem is:

* fix code in hostap to also use alloc_netdev()

* bring back "manual" allocation of PSDevice structure
 
* use private netdev areas only to store the pointer to the single PSDevice
  structure that we allocate "manually" (requires converting all netdev_priv()
  instances to *netdev_priv() etc but the change should be straightforward).