| Message ID | 200907302317.52648.bzolnier@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
Bartlomiej Zolnierkiewicz wrote: > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Subject: [PATCH] pcnet32: VLB support fixes > > VLB support has been broken since at least 2004-2005 period as some > changes introduced back then assumed that ->pci_dev is always valid, > lets try to fix it: > > - remove duplicated SET_NETDEV_DEV() call > > - call SET_NETDEV_DEV() only for PCI devices > > - check for ->pci_dev validity in pcnet32_open() > > [ Alternatively we may consider removing VLB support but there would not > be much gain in it since an extra driver code needed for VLB support is > minimal and quite simple. ] > > This takes care of the following entry from Dan's list: > > drivers/net/pcnet32.c +1889 pcnet32_probe1(298) warning: variable derefenced before check 'pdev' > > Reported-by: Dan Carpenter <error27@gmail.com> > Cc: corbet@lwn.net > Cc: eteo@redhat.com > Cc: Julia Lawall <julia@diku.dk> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > --- > PS I still keep the original cc: list from the smatch thread -- please let > me know if you don't want to be spammed.. ;-) > > drivers/net/pcnet32.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) ACK -- the patch is correct AFAICS -- but would it not be better to have a struct device for the VLB device? We have capability to create struct device for isa and eisa, so VLB is quite doable. Jeff, who converted old-ISDN ISA+EISA+PCI drivers to hotplug model, once upon a time -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 July 2009 23:32:19 Jeff Garzik wrote: > Bartlomiej Zolnierkiewicz wrote: > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > Subject: [PATCH] pcnet32: VLB support fixes > > > > VLB support has been broken since at least 2004-2005 period as some > > changes introduced back then assumed that ->pci_dev is always valid, > > lets try to fix it: > > > > - remove duplicated SET_NETDEV_DEV() call > > > > - call SET_NETDEV_DEV() only for PCI devices > > > > - check for ->pci_dev validity in pcnet32_open() > > > > [ Alternatively we may consider removing VLB support but there would not > > be much gain in it since an extra driver code needed for VLB support is > > minimal and quite simple. ] > > > > This takes care of the following entry from Dan's list: > > > > drivers/net/pcnet32.c +1889 pcnet32_probe1(298) warning: variable derefenced before check 'pdev' > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > Cc: corbet@lwn.net > > Cc: eteo@redhat.com > > Cc: Julia Lawall <julia@diku.dk> > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > --- > > PS I still keep the original cc: list from the smatch thread -- please let > > me know if you don't want to be spammed.. ;-) > > > > drivers/net/pcnet32.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > ACK -- the patch is correct AFAICS -- but would it not be better to have > a struct device for the VLB device? Sure, such addition would be needed for sysfs support on VLB devices.. > We have capability to create struct device for isa and eisa, so VLB is > quite doable. > > Jeff, who converted old-ISDN ISA+EISA+PCI drivers to > hotplug model, once upon a time ..and it seems that you've the needed experience! :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz wrote: > On Thursday 30 July 2009 23:32:19 Jeff Garzik wrote: >> Bartlomiej Zolnierkiewicz wrote: >>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> >>> Subject: [PATCH] pcnet32: VLB support fixes >>> >>> VLB support has been broken since at least 2004-2005 period as some >>> changes introduced back then assumed that ->pci_dev is always valid, >>> lets try to fix it: >>> >>> - remove duplicated SET_NETDEV_DEV() call >>> >>> - call SET_NETDEV_DEV() only for PCI devices >>> >>> - check for ->pci_dev validity in pcnet32_open() >>> >>> [ Alternatively we may consider removing VLB support but there would not >>> be much gain in it since an extra driver code needed for VLB support is >>> minimal and quite simple. ] >>> >>> This takes care of the following entry from Dan's list: >>> >>> drivers/net/pcnet32.c +1889 pcnet32_probe1(298) warning: variable derefenced before check 'pdev' >>> >>> Reported-by: Dan Carpenter <error27@gmail.com> >>> Cc: corbet@lwn.net >>> Cc: eteo@redhat.com >>> Cc: Julia Lawall <julia@diku.dk> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> >>> --- >>> PS I still keep the original cc: list from the smatch thread -- please let >>> me know if you don't want to be spammed.. ;-) >>> >>> drivers/net/pcnet32.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >> ACK -- the patch is correct AFAICS -- but would it not be better to have >> a struct device for the VLB device? > > Sure, such addition would be needed for sysfs support on VLB devices.. >> We have capability to create struct device for isa and eisa, so VLB is >> quite doable. >> >> Jeff, who converted old-ISDN ISA+EISA+PCI drivers to >> hotplug model, once upon a time > > ..and it seems that you've the needed experience! :) <grin> In my esteemed experience, I point you to the 39-line include/linux/isa.h as a starting point for grep... :) Jeff -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 July 2009 23:58:00 Jeff Garzik wrote: > Bartlomiej Zolnierkiewicz wrote: > > On Thursday 30 July 2009 23:32:19 Jeff Garzik wrote: > >> Bartlomiej Zolnierkiewicz wrote: > >>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > >>> Subject: [PATCH] pcnet32: VLB support fixes > >>> > >>> VLB support has been broken since at least 2004-2005 period as some > >>> changes introduced back then assumed that ->pci_dev is always valid, > >>> lets try to fix it: > >>> > >>> - remove duplicated SET_NETDEV_DEV() call > >>> > >>> - call SET_NETDEV_DEV() only for PCI devices > >>> > >>> - check for ->pci_dev validity in pcnet32_open() > >>> > >>> [ Alternatively we may consider removing VLB support but there would not > >>> be much gain in it since an extra driver code needed for VLB support is > >>> minimal and quite simple. ] > >>> > >>> This takes care of the following entry from Dan's list: > >>> > >>> drivers/net/pcnet32.c +1889 pcnet32_probe1(298) warning: variable derefenced before check 'pdev' > >>> > >>> Reported-by: Dan Carpenter <error27@gmail.com> > >>> Cc: corbet@lwn.net > >>> Cc: eteo@redhat.com > >>> Cc: Julia Lawall <julia@diku.dk> > >>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > >>> --- > >>> PS I still keep the original cc: list from the smatch thread -- please let > >>> me know if you don't want to be spammed.. ;-) > >>> > >>> drivers/net/pcnet32.c | 12 +++++++----- > >>> 1 file changed, 7 insertions(+), 5 deletions(-) > >> ACK -- the patch is correct AFAICS -- but would it not be better to have > >> a struct device for the VLB device? > > > > Sure, such addition would be needed for sysfs support on VLB devices.. > > > >> We have capability to create struct device for isa and eisa, so VLB is > >> quite doable. > >> > >> Jeff, who converted old-ISDN ISA+EISA+PCI drivers to > >> hotplug model, once upon a time > > > > ..and it seems that you've the needed experience! :) > > <grin> In my esteemed experience, I point you to the 39-line > include/linux/isa.h as a starting point for grep... :) Send patches. I don't have a time/motivation for enhancing VLB support, not even mentioning the access to hardware needed to test such changes.. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Date: Thu, 30 Jul 2009 23:17:52 +0200 > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Subject: [PATCH] pcnet32: VLB support fixes > > VLB support has been broken since at least 2004-2005 period as some > changes introduced back then assumed that ->pci_dev is always valid, > lets try to fix it: > > - remove duplicated SET_NETDEV_DEV() call > > - call SET_NETDEV_DEV() only for PCI devices > > - check for ->pci_dev validity in pcnet32_open() > > [ Alternatively we may consider removing VLB support but there would not > be much gain in it since an extra driver code needed for VLB support is > minimal and quite simple. ] > > This takes care of the following entry from Dan's list: > > drivers/net/pcnet32.c +1889 pcnet32_probe1(298) warning: variable derefenced before check 'pdev' > > Reported-by: Dan Carpenter <error27@gmail.com> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Also applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: b/drivers/net/pcnet32.c =================================================================== --- a/drivers/net/pcnet32.c +++ b/drivers/net/pcnet32.c @@ -1719,7 +1719,9 @@ pcnet32_probe1(unsigned long ioaddr, int ret = -ENOMEM; goto err_release_region; } - SET_NETDEV_DEV(dev, &pdev->dev); + + if (pdev) + SET_NETDEV_DEV(dev, &pdev->dev); if (pcnet32_debug & NETIF_MSG_PROBE) printk(KERN_INFO PFX "%s at %#3lx,", chipname, ioaddr); @@ -1818,7 +1820,6 @@ pcnet32_probe1(unsigned long ioaddr, int spin_lock_init(&lp->lock); - SET_NETDEV_DEV(dev, &pdev->dev); lp->name = chipname; lp->shared_irq = shared; lp->tx_ring_size = TX_RING_SIZE; /* default tx ring size */ @@ -2089,6 +2090,7 @@ static void pcnet32_free_ring(struct net static int pcnet32_open(struct net_device *dev) { struct pcnet32_private *lp = netdev_priv(dev); + struct pci_dev *pdev = lp->pci_dev; unsigned long ioaddr = dev->base_addr; u16 val; int i; @@ -2149,9 +2151,9 @@ static int pcnet32_open(struct net_devic lp->a.write_csr(ioaddr, 124, val); /* Allied Telesyn AT 2700/2701 FX are 100Mbit only and do not negotiate */ - if (lp->pci_dev->subsystem_vendor == PCI_VENDOR_ID_AT && - (lp->pci_dev->subsystem_device == PCI_SUBDEVICE_ID_AT_2700FX || - lp->pci_dev->subsystem_device == PCI_SUBDEVICE_ID_AT_2701FX)) { + if (pdev && pdev->subsystem_vendor == PCI_VENDOR_ID_AT && + (pdev->subsystem_device == PCI_SUBDEVICE_ID_AT_2700FX || + pdev->subsystem_device == PCI_SUBDEVICE_ID_AT_2701FX)) { if (lp->options & PCNET32_PORT_ASEL) { lp->options = PCNET32_PORT_FD | PCNET32_PORT_100; if (netif_msg_link(lp))