LinuxLists.cc - Staging: vt6656 ?

2009-06-09 10:39:42

Subject: Staging: vt6656 ?

Hi Greg, Forest

Are you going to merge driver for VT6656 also?

I have a limited access to this hardware and can help to cleanup it.

http://www.viaarena.com/Driver/VT6656_Linux_src_v1.19_12_x86.zip

2009-06-09 10:57:39

by Greg KH

[permalink] [raw]

Subject: Re: Staging: vt6656 ?

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.

thanks,

greg k-h

2009-06-09 11:43:24

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.

Let me know if you think I ought to do something else with them.

Thanks,
Forest
--
Forest Bond
http://www.alittletooquiet.net
http://www.pytagsfs.org

Attachments:

(No filename) (1.25 kB)
signature.asc (189.00 B)
Digital signature Download all attachments

2009-06-28 15:53:55

by Bartlomiej Zolnierkiewicz

[permalink] [raw]

Subject: Re: Staging: vt6656 ?

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 <[email protected]>
Subject: [PATCH] vt6655: add TODO

Cc: Forest Bond <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
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 <[email protected]>,
+Forest Bond <[email protected]> and Bartlomiej Zolnierkiewicz
+<[email protected]>.

and the corresponding one for vt6656:

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] vt6656: add TODO

Cc: Forest Bond <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
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 <[email protected]>,
+Forest Bond <[email protected]> and Bartlomiej Zolnierkiewicz
+<[email protected]>.

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

2009-07-02 18:22:18

by Forest Bond

[permalink] [raw]

Subject: Re: Staging: vt6656 ?

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?

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

Agreed. I gave this a best effort, but was not very confident about the result.

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

Thanks,
Forest
--
Forest Bond
http://www.alittletooquiet.net
http://www.pytagsfs.org

Attachments:

(No filename) (2.47 kB)
signature.asc (189.00 B)
Digital signature Download all attachments

2009-07-06 15:27:14

by Bartlomiej Zolnierkiewicz

[permalink] [raw]

Subject: Re: Staging: vt6656 ?

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