Re: Staging: vt6656 ?

Bartlomiej Zolnierkiewicz
2009-07-02T17:57:32Z

Hi,

On Sunday 28 June 2009 18:47:16 Forest Bond wrote:
> Hi,
>
> On Sun, Jun 28, 2009 at 05:59:45PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 28 June 2009 15:57:31 Forest Bond wrote:
> > > Hi,
> > >
> > > On Sun, Jun 28, 2009 at 03:43:12PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tuesday 09 June 2009 13:33:43 Forest Bond wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, Jun 09, 2009 at 03:51:01AM -0700, Greg KH wrote:
> > > > > > On Tue, Jun 09, 2009 at 02:31:30PM +0400, Alexander Beregalov wrote:
> > > > > > > Hi Greg, Forest
> > > > > > >
> > > > > > > Are you going to merge driver for VT6656 also?
> > > > > >
> > > > > > If someone sends me some patches for it, sure, I will.
> > > > >
> > > > > I have patches almost ready. I'll send them by the end of next week at the
> > > > > latest. Busy week here.
> > > >
> > > > Did I miss the patch? There are people already doing cleanups for VT6655
> > > > driver (which seems to share a great deal of code with VT6656 driver) so
> > > > it would greatly help to get VT6656 merged ASAP and merge the shared code
> > > > first to not duplicate efforts.
> > > >
> > > > Also if you need some help with integrating the driver or hosting patches
> > > > in git tree at kernel.org before Greg picks them up [ he seems to be buried
> > > > alive by patches at the moment :) ] I'll be happy to help..
> > >
> > > I sent it to Greg about two weeks ago. I assume it is in his queue somewhere.
> >
> > There seems to be a lot of stuff in his queue.. ;)
> >
> > > Let me know if you think I ought to do something else with them.
> >
> > Please re-post with cc:ing linux-kernel so people looking for them (i.e. me)
> > can pick them up from the list if needed (please also cc: lkml on all patches
> > in the future, thanks!).
>
> Okay. The first patch is quite large, so I will compress it.
>
> > [ 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.. :)

I also took a look at both drivers and unification is certainly possible
and desirable, though not as easy as I had hoped initially.. here is the
sketch of preliminary TODO for vt6655:

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] vt6655: add TODO

Cc: Forest Bond <forest@alittletooquiet.net>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/staging/vt6655/TODO | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

Index: b/drivers/staging/vt6655/TODO
===================================================================
--- /dev/null
+++ b/drivers/staging/vt6655/TODO
@@ -0,0 +1,21 @@
+TODO:
+- remove __cplusplus ifdefs
+- prepare for merge with vt6656 driver:
+ - rename DEVICE_PRT() to DBG_PRT()
+ - share 80211*.h includes
+ - move code for channel mapping from card.c to channel.c
+ - split rf.c
+ - remove dead code
+ - abstract VT3253 chipset specific code
+- add common vt665x infrastructure
+- kill ttype.h
+- switch to use LIB80211
+- switch to use MAC80211
+- use kernel coding style
+- checkpatch.pl fixes
+- sparse fixes
+- integrate with drivers/net/wireless
+
+Please send any patches to Greg Kroah-Hartman <greg@kroah.com>,
+Forest Bond <forest@alittletooquiet.net> and Bartlomiej Zolnierkiewicz
+<bzolnier@gmail.com>.

and the corresponding one for vt6656:

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] vt6656: add TODO

Cc: Forest Bond <forest@alittletooquiet.net>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/staging/vt6656/TODO | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

Index: b/drivers/staging/vt6656/TODO
===================================================================
--- /dev/null
+++ b/drivers/staging/vt6656/TODO
@@ -0,0 +1,20 @@
+TODO:
+- remove __cplusplus ifdefs
+- remove kernel version compatibility wrappers
+- remove support for older wireless extensions
+- prepare for merge with vt6655 driver:
+ - remove PRINT_K() macro
+ - split rf.c
+ - abstract VT3184 chipset specific code
+- add common vt665x infrastructure
+- kill ttype.h
+- switch to use LIB80211
+- switch to use MAC80211
+- use kernel coding style
+- checkpatch.pl fixes
+- sparse fixes
+- integrate with drivers/net/wireless
+
+Please send any patches to Greg Kroah-Hartman <greg@kroah.com>,
+Forest Bond <forest@alittletooquiet.net> and Bartlomiej Zolnierkiewicz
+<bzolnier@gmail.com>.

> 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:

--- a/drivers/staging/vt6655/hostap.c
+++ b/drivers/staging/vt6655/hostap.c
@@ -100,6 +100,7 @@ static int msglevel =MSG_LEVEL_INFO;

static int hostap_enable_hostapd(PSDevice pDevice, int rtnl_locked)
{
+ PSDevice apdev_priv;
struct net_device *dev = pDevice->dev;
int ret;

@@ -124,12 +125,13 @@ static int hostap_enable_hostapd(PSDevice pDevice, int rtnl_locked)
dev->name, pDevice->apdev->name);

#else
- pDevice->apdev = (struct net_device *)kmalloc(sizeof(struct net_device), GFP_KERNEL);
+ pDevice->apdev = (struct net_device *)kmalloc(sizeof(struct net_device), GFP_KERNEL);
if (pDevice->apdev == NULL)
return -ENOMEM;
memset(pDevice->apdev, 0, sizeof(struct net_device));

- pDevice->apdev->priv = pDevice;
+ apdev_priv = netdev_priv(pDevice->apdev);
+ *apdev_priv = *pDevice;

We don't use alloc_netdev() for apdev allocation so by using netdev_priv()
later we are potentially accessing unrelated memory part.

--- 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.

[ I don't know whether this is a problem currently but it looks suspicious. ]