Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

[Greg KH]

On Fri, Dec 09, 2016 at 12:05:53AM +, KY Srinivasan wrote:
> 
> 
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Thursday, December 8, 2016 7:56 AM
> > To: KY Srinivasan 
> > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > o...@aepfle.de; a...@canonical.com; vkuzn...@redhat.com;
> > jasow...@redhat.com; leann.ogasaw...@canonical.com;
> > bjorn.helg...@gmail.com; Haiyang Zhang 
> > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial
> > numbers
> > 
> > On Thu, Dec 08, 2016 at 12:33:43AM -0800, k...@exchange.microsoft.com
> > wrote:
> > > From: Haiyang Zhang 
> > >
> > > We currently use MAC address to match VF and synthetic NICs. Hyper-V
> > > provides a serial number to both devices for this purpose. This patch
> > > implements the matching based on VF serial numbers. This is the way
> > > specified by the protocol and more reliable.
> > >
> > > Signed-off-by: Haiyang Zhang 
> > > Signed-off-by: K. Y. Srinivasan 
> > > ---
> > >  drivers/net/hyperv/netvsc_drv.c |   55
> > ---
> > >  1 files changed, 51 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c
> > > index 9522763..c5778cf 100644
> > > --- a/drivers/net/hyperv/netvsc_drv.c
> > > +++ b/drivers/net/hyperv/netvsc_drv.c
> > > @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct
> > net_device *netdev)
> > >   free_netdev(netdev);
> > >  }
> > >
> > > -static struct net_device *get_netvsc_bymac(const u8 *mac)
> > > +static struct net_device *get_netvsc_byvfser(u32 vfser)
> > >  {
> > >   struct net_device *dev;
> > > + struct net_device_context *ndev_ctx;
> > >
> > >   ASSERT_RTNL();
> > >
> > > @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct net_device
> > *netdev)
> > >   if (dev->netdev_ops != _ops)
> > >   continue;   /* not a netvsc device */
> > >
> > > - if (ether_addr_equal(mac, dev->perm_addr))
> > > + ndev_ctx = netdev_priv(dev);
> > > + if (ndev_ctx->vf_serial == vfser)
> > >   return dev;
> > >   }
> > >
> > > @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct
> > net_device *netdev)
> > >   return NULL;
> > >  }
> > >
> > > +static u32 netvsc_get_vfser(struct net_device *vf_netdev)
> > > +{
> > > + struct device *dev;
> > > + struct hv_device *hdev;
> > > + struct hv_pcibus_device *hbus = NULL;
> > > + struct list_head *iter;
> > > + struct hv_pci_dev *hpdev;
> > > + unsigned long flags;
> > > + u32 vfser = 0;
> > > + u32 count = 0;
> > > +
> > > + for (dev = _netdev->dev; dev; dev = dev->parent) {
> > 
> > You are going to walk the whole device tree backwards?  That's crazy.
> > And foolish.  And racy and broken (what happens if the tree changes
> > while you do this?)  Where is the lock being grabbed while this happens?
> > What about reference counts?  Do you see other drivers ever doing this
> > (if you do, point them out and I'll go yell at them too...)
> 
> Greg,
> 
> We are registering for netdev events. Coming into this function, the caller
> guarantees that the list of netdevs does not change - we assert this on entry:
> ASSERT_RTNL(). We are only walking up the device tree for the netdevs whose
> state change is being notified to us - the device tree being walked here is 
> limited to
> netdevs under question. 

But a netdev is a child of some type of "real" device, and you are now
walking the tree of all devices up to the "root" parent device, which
means you will hit PCI bridges, USB controllers, and all sorts of fun
things if you are a child of those types of devices.

And can't you tell if the netdev for this event, really is "your"
netdev?  Or are you getting called this for "all" netdevs?  Sorry, I
don't know this api, any pointers to it would be appreciated.

> We have a reference to the device and we know the device is not going away. 
> Is it not
> safe to dereference the parent pointer - after all the child has taken a 
> reference on
> the parent as part of  device_add() call.

It might be, and might not be.  There's a reason you don't see this
pattern anywhere in the kernel because of this...

> > > + if (!dev_is_vmbus(dev))
> > > + continue;
> > 
> > Ick.
> > 
> > Why isn't your parent pointer a vmbus device all the time?  How could
> > you get burried down in the device hierarchy when you are the driver for
> > a specific bus type in the first place?  How could this function ever be
> > called for a device that is NOT of this type?
> 
> We get notified when state changes on any of the netdev devices in the system.
> Not all netdevs in the system belong to vmbus. Consider for instance the 
> emulated NIC that can be configured. This is an emulated PCI NIC. We are only
> interested in 

Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging

[Greg KH]

On Fri, Dec 09, 2016 at 12:36:26AM +, Stuart Yoder wrote:
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Thursday, December 08, 2016 10:05 AM
> > To: Stuart Yoder 
> > Cc: de...@driverdev.osuosl.org; ag...@suse.de; a...@arndb.de; 
> > linux-ker...@vger.kernel.org; Leo Li
> > ; Catalin Horghidan ; Ioana 
> > Ciornei
> > ; Laurentiu Tudor 
> > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
> > 
> > On Wed, Dec 07, 2016 at 08:19:20PM +, Stuart Yoder wrote:
> > > > -Original Message-
> > > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > > Sent: Wednesday, December 07, 2016 9:53 AM
> > > > To: Stuart Yoder 
> > > > Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; 
> > > > ag...@suse.de; a...@arndb.de; Leo Li
> > > > ; Ioana Ciornei ; Catalin 
> > > > Horghidan
> > > > ; Laurentiu Tudor ; 
> > > > Ruxandra Ioana Radulescu
> > > > 
> > > > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of 
> > > > staging
> > > >
> > > > On Thu, Dec 01, 2016 at 04:41:26PM -0600, Stuart Yoder wrote:
> > > > > Move the source files out of staging into their final locations:
> > > > >   -include files in drivers/staging/fsl-mc/include go to 
> > > > > include/linux/fsl
> > > > >   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
> > > > >   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
> > > > >   -README.txt, providing and overview of DPAA goes to
> > > > >Documentation/dpaa2/overview.txt
> > > > >   -update MAINTAINERS with new location
> > > > >
> > > > > Delete other remaining staging files-- Makefile, Kconfig, TODO
> > > >
> > > > Ok, given that I haven't ever reviewed this code, I had a few questions
> > > > that I couldn't easily figure out by looking at your code:
> > > > - what is the lifecycle of your 'struct device' usage?  Who
> > > >   creates it, who frees it, and who accesses it?
> > >
> > > We embed a 'struct device' inside our bus specific device struct
> > > 'struct fsl_mc_device'.  So, when a new fsl-mc object is discovered
> > > on the bus during initial enumeration or hotplug we create a new
> > > 'struct fsl_mc_device' and do a device_initialize()/device_add().
> > > (see fsl_mc_device_add() for where this is done)
> > >
> > > 'struct device' is freed when a device is removed-- the reverse
> > > of the above.
> > 
> > Where is the device freed?  I see you trying to do some "odd" stuff in
> > fsl_mc_device_remove() by deleting and then putting a device structure.
> > I can't find a "release()" callback anywhere for your bus, where is it?
> > 
> > What happens when the reference count falls to 0 for your struct device?
> 
> Hrm...something seems wrong in free path, and I think this needs to
> be refactored.
> 
> IIRC, when German (former maintainer) wrote that code he loosely based
> it on the register/unregister platform bus code:
> 
> int platform_device_register(struct platform_device *pdev)
> {
> device_initialize(>dev);
> arch_setup_pdev_archdata(pdev);
> return platform_device_add(pdev);
> }
> void platform_device_unregister(struct platform_device *pdev)
> {
> platform_device_del(pdev);
> platform_device_put(pdev);
> }
> 
> ...I'm puzzling over how that code handles a refcount of zero
> as I see no 'release' callback anywhere, but I must be missing
> something.
> 
> In any case, we'll get this refactored.

Have you tried removing a device?  The kernel should complain loudly
about there not being a release function for your device.

> > > > - root_dprc_count, why are you using an atomic variable for
> > > >   this?  What is it for other than "look, I'm running!"?
> > >
> > > There can be multiple root buses, and this variable simply tracks the 
> > > count
> > > of them.
> > 
> > Why does it matter?
> > 
> > > It's is atomic there might be a theoretical race condition where 2
> > > buses might be added at the same time.  The root buses are found in
> > > the device tree and so if there is no chance that device tree
> > > processing happens in parallel on multiple cores then we could remove
> > > the atomic.
> > 
> > Why not just use a lock, or better yet, not care about a "count" at all?
> > I don't see you doing anything with the count, other than emitting a
> > WARN() if it drops down below 0 for some reason, or when you call
> > fsl_mc_bus_exists() which for some reason is exported yet no one uses
> > it...
> 
> We can drop this count.  At one time I think there was envisioned an 
> external user who needed it, but it's no longer the case.

Please do, we are trying to get rid of atomic_t abuse on other mailing
lists, 

Re: [PATCH] Staging: ks7010: ks7010_sdio.h: Fixed coding style errors

[Greg KH]

On Fri, Dec 09, 2016 at 12:29:21PM +, Manoj Sawai wrote:
> Errors - Complex macro not a parentheses and trailing whitespace
> Also fixed other small checkpatch warnings and checks.

If you ever say "also" in a changelog, that's a huge hint that the patch
needs to be broken up into multiple patches.  That is the case here,
please only do one type of coding style fix at a time.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] Staging: ks7010: ks7010_sdio.h: Fixed coding style errors

[Manoj Sawai]

Errors - Complex macro not a parentheses and trailing whitespace
Also fixed other small checkpatch warnings and checks.

Signed-off-by: Manoj Sawai 
---
 drivers/staging/ks7010/ks7010_sdio.h | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.h 
b/drivers/staging/ks7010/ks7010_sdio.h
index 0f5fd848e23d..f12d41835b03 100644
--- a/drivers/staging/ks7010/ks7010_sdio.h
+++ b/drivers/staging/ks7010/ks7010_sdio.h
@@ -46,7 +46,7 @@
  */
 #define WSTATUS_RSIZE  0x14
 #define WSTATUS_MASK   0x80/* Write Status Register value */
-#define RSIZE_MASK 0x7F/* Read Data Size Register value [10:4] 
*/
+#define RSIZE_MASK 0x7F// Read Data Size Register value [10:4]
 
 /* ARM to SD interrupt Enable */
 #define INT_ENABLE 0x20
@@ -81,11 +81,11 @@
 
 /* AHB Data Window  0x01-0x01 */
 #define DATA_WINDOW0x01
-#define WINDOW_SIZE64*1024
+#define WINDOW_SIZE(64 * 1024)
 
 #define KS7010_IRAM_ADDRESS0x0600
 
-/* 
+/*
  * struct define
  */
 struct hw_info_t {
@@ -115,7 +115,7 @@ struct ks_sdio_card {
 struct tx_device_buffer {
unsigned char *sendp;   /* pointer of send req data */
unsigned int size;
-   void (*complete_handler) (void *arg1, void *arg2);
+   void (*complete_handler)(void *arg1, void *arg2);
void *arg1;
void *arg2;
 };
@@ -142,6 +142,7 @@ struct rx_device {
unsigned int qtail; /* rx buffer queue last pointer */
spinlock_t rx_dev_lock;
 };
+
 #defineROM_FILE "ks7010sd.rom"
 
 #endif /* _KS7010_SDIO_H */
-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: i4l :fixed coding style

[Tabrez khan]

Remove braces {} for single if statement block.

Signed-off-by: Tabrez khan 
---
 drivers/staging/i4l/act2000/module.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/i4l/act2000/module.c 
b/drivers/staging/i4l/act2000/module.c
index 99c9c0a..fc14de4 100644
--- a/drivers/staging/i4l/act2000/module.c
+++ b/drivers/staging/i4l/act2000/module.c
@@ -372,9 +372,8 @@ act2000_command(act2000_card *card, isdn_ctrl *c)
if (!(chan = find_channel(card, c->arg & 0x0f)))
break;
if (strlen(c->parm.num)) {
-   if (card->ptype == ISDN_PTYPE_EURO) {
+   if (card->ptype == ISDN_PTYPE_EURO)
chan->eazmask = act2000_find_msn(card, 
c->parm.num, 0);
-   }
if (card->ptype == ISDN_PTYPE_1TR6) {
int i;
chan->eazmask = 0;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging

[Stuart Yoder]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, December 08, 2016 10:05 AM
> To: Stuart Yoder 
> Cc: de...@driverdev.osuosl.org; ag...@suse.de; a...@arndb.de; 
> linux-ker...@vger.kernel.org; Leo Li
> ; Catalin Horghidan ; Ioana 
> Ciornei
> ; Laurentiu Tudor 
> Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
> 
> On Wed, Dec 07, 2016 at 08:19:20PM +, Stuart Yoder wrote:
> > > -Original Message-
> > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > Sent: Wednesday, December 07, 2016 9:53 AM
> > > To: Stuart Yoder 
> > > Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; 
> > > ag...@suse.de; a...@arndb.de; Leo Li
> > > ; Ioana Ciornei ; Catalin 
> > > Horghidan
> > > ; Laurentiu Tudor ; 
> > > Ruxandra Ioana Radulescu
> > > 
> > > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of 
> > > staging
> > >
> > > On Thu, Dec 01, 2016 at 04:41:26PM -0600, Stuart Yoder wrote:
> > > > Move the source files out of staging into their final locations:
> > > >   -include files in drivers/staging/fsl-mc/include go to 
> > > > include/linux/fsl
> > > >   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
> > > >   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
> > > >   -README.txt, providing and overview of DPAA goes to
> > > >Documentation/dpaa2/overview.txt
> > > >   -update MAINTAINERS with new location
> > > >
> > > > Delete other remaining staging files-- Makefile, Kconfig, TODO
> > >
> > > Ok, given that I haven't ever reviewed this code, I had a few questions
> > > that I couldn't easily figure out by looking at your code:
> > >   - what is the lifecycle of your 'struct device' usage?  Who
> > > creates it, who frees it, and who accesses it?
> >
> > We embed a 'struct device' inside our bus specific device struct
> > 'struct fsl_mc_device'.  So, when a new fsl-mc object is discovered
> > on the bus during initial enumeration or hotplug we create a new
> > 'struct fsl_mc_device' and do a device_initialize()/device_add().
> > (see fsl_mc_device_add() for where this is done)
> >
> > 'struct device' is freed when a device is removed-- the reverse
> > of the above.
> 
> Where is the device freed?  I see you trying to do some "odd" stuff in
> fsl_mc_device_remove() by deleting and then putting a device structure.
> I can't find a "release()" callback anywhere for your bus, where is it?
> 
> What happens when the reference count falls to 0 for your struct device?

Hrm...something seems wrong in free path, and I think this needs to
be refactored.

IIRC, when German (former maintainer) wrote that code he loosely based
it on the register/unregister platform bus code:

int platform_device_register(struct platform_device *pdev)
{
device_initialize(>dev);
arch_setup_pdev_archdata(pdev);
return platform_device_add(pdev);
}
void platform_device_unregister(struct platform_device *pdev)
{
platform_device_del(pdev);
platform_device_put(pdev);
}

...I'm puzzling over how that code handles a refcount of zero
as I see no 'release' callback anywhere, but I must be missing
something.

In any case, we'll get this refactored.

> > >   - root_dprc_count, why are you using an atomic variable for
> > > this?  What is it for other than "look, I'm running!"?
> >
> > There can be multiple root buses, and this variable simply tracks the count
> > of them.
> 
> Why does it matter?
> 
> > It's is atomic there might be a theoretical race condition where 2
> > buses might be added at the same time.  The root buses are found in
> > the device tree and so if there is no chance that device tree
> > processing happens in parallel on multiple cores then we could remove
> > the atomic.
> 
> Why not just use a lock, or better yet, not care about a "count" at all?
> I don't see you doing anything with the count, other than emitting a
> WARN() if it drops down below 0 for some reason, or when you call
> fsl_mc_bus_exists() which for some reason is exported yet no one uses
> it...

We can drop this count.  At one time I think there was envisioned an 
external user who needed it, but it's no longer the case.

Given the additional refactoring, I think the fsl-mc bus driver needs
to stay in staging for a bit.  In order to facilitate further review
I'm going to refactor the patch series:
  staging: fsl-mc: move bus driver out of staging, add dpio

...to just add dpio (into staging).  This will allow the Eth driver
series sent earlier this week to go into staging:
  staging: Introduce Freescale DPAA2 Ethernet driver

With all that in staging we'll have a fully functional Ethernet
driver.

Thanks,
Stuart

RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

[KY Srinivasan]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, December 8, 2016 7:56 AM
> To: KY Srinivasan 
> Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> o...@aepfle.de; a...@canonical.com; vkuzn...@redhat.com;
> jasow...@redhat.com; leann.ogasaw...@canonical.com;
> bjorn.helg...@gmail.com; Haiyang Zhang 
> Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial
> numbers
> 
> On Thu, Dec 08, 2016 at 12:33:43AM -0800, k...@exchange.microsoft.com
> wrote:
> > From: Haiyang Zhang 
> >
> > We currently use MAC address to match VF and synthetic NICs. Hyper-V
> > provides a serial number to both devices for this purpose. This patch
> > implements the matching based on VF serial numbers. This is the way
> > specified by the protocol and more reliable.
> >
> > Signed-off-by: Haiyang Zhang 
> > Signed-off-by: K. Y. Srinivasan 
> > ---
> >  drivers/net/hyperv/netvsc_drv.c |   55
> ---
> >  1 files changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> b/drivers/net/hyperv/netvsc_drv.c
> > index 9522763..c5778cf 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct
> net_device *netdev)
> > free_netdev(netdev);
> >  }
> >
> > -static struct net_device *get_netvsc_bymac(const u8 *mac)
> > +static struct net_device *get_netvsc_byvfser(u32 vfser)
> >  {
> > struct net_device *dev;
> > +   struct net_device_context *ndev_ctx;
> >
> > ASSERT_RTNL();
> >
> > @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct net_device
> *netdev)
> > if (dev->netdev_ops != _ops)
> > continue;   /* not a netvsc device */
> >
> > -   if (ether_addr_equal(mac, dev->perm_addr))
> > +   ndev_ctx = netdev_priv(dev);
> > +   if (ndev_ctx->vf_serial == vfser)
> > return dev;
> > }
> >
> > @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct
> net_device *netdev)
> > return NULL;
> >  }
> >
> > +static u32 netvsc_get_vfser(struct net_device *vf_netdev)
> > +{
> > +   struct device *dev;
> > +   struct hv_device *hdev;
> > +   struct hv_pcibus_device *hbus = NULL;
> > +   struct list_head *iter;
> > +   struct hv_pci_dev *hpdev;
> > +   unsigned long flags;
> > +   u32 vfser = 0;
> > +   u32 count = 0;
> > +
> > +   for (dev = _netdev->dev; dev; dev = dev->parent) {
> 
> You are going to walk the whole device tree backwards?  That's crazy.
> And foolish.  And racy and broken (what happens if the tree changes
> while you do this?)  Where is the lock being grabbed while this happens?
> What about reference counts?  Do you see other drivers ever doing this
> (if you do, point them out and I'll go yell at them too...)

Greg,

We are registering for netdev events. Coming into this function, the caller
guarantees that the list of netdevs does not change - we assert this on entry:
ASSERT_RTNL(). We are only walking up the device tree for the netdevs whose
state change is being notified to us - the device tree being walked here is 
limited to
netdevs under question. 

We have a reference to the device and we know the device is not going away. Is 
it not
safe to dereference the parent pointer - after all the child has taken a 
reference on
the parent as part of  device_add() call.
 
> 
> > +   if (!dev_is_vmbus(dev))
> > +   continue;
> 
> Ick.
> 
> Why isn't your parent pointer a vmbus device all the time?  How could
> you get burried down in the device hierarchy when you are the driver for
> a specific bus type in the first place?  How could this function ever be
> called for a device that is NOT of this type?

We get notified when state changes on any of the netdev devices in the system.
Not all netdevs in the system belong to vmbus. Consider for instance the 
emulated NIC that can be configured. This is an emulated PCI NIC. We are only
interested in netdevs that correspond to the VF instance that we are interested 
in.

Regards,

K. Y
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: sm750fb: update license

[Sudip Mukherjee]

On Saturday 26 November 2016 11:02 AM, Greg Kroah-Hartman wrote:

On Sat, Nov 26, 2016 at 09:07:42AM +, Sudip Mukherjee wrote:

The driver was actually released with BSD license. It also gained GPL
when it was submitted to be included in the kernel.

Cc: Teddy Wang 
Cc: gzhou1 
Signed-off-by: Sudip Mukherjee 
---
  drivers/staging/sm750fb/sm750.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 2d22c51..e9632f1 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -1243,4 +1243,4 @@ static void __exit lynxfb_exit(void)
  MODULE_AUTHOR("monk liu ");
  MODULE_AUTHOR("Sudip Mukherjee ");
  MODULE_DESCRIPTION("Frame buffer driver for SM750 chipset");
-MODULE_LICENSE("GPL v2");
+MODULE_LICENSE("Dual BSD/GPL");


Should you also update the top of the files that don't say anything
about licenses?

I'll take this patch, but an update for them would also be good.


Sure, I will update the pages with the license text that Teddy sent me 
and will send you the patch by this weekend.


Regards
Sudip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: lustre: Fix variable type declaration after refactoring

[Quentin Lambert]

A recent clean-up declared och_flags as a int rather than fmode_t. This
lead to the following sparse warning:

drivers/staging/lustre/lustre/llite/file.c:106:30: warning: restricted
fmode_t degrades to integer

This patch fixes this issue.

Fixes: 0a1200991234f7 ("staging: lustre: cleanup lustre_lib.h")
Signed-off-by: Quentin Lambert 
---
 v2: fixes the referenced sha

 drivers/staging/lustre/lustre/include/obd.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/staging/lustre/lustre/include/obd.h
+++ b/drivers/staging/lustre/lustre/include/obd.h
@@ -889,7 +889,7 @@ struct obd_client_handle {
struct md_open_data *och_mod;
struct lustre_handle och_lease_handle; /* open lock for lease */
__u32och_magic;
-   int  och_flags;
+   fmode_t  och_flags;
 };
 
 #define OBD_CLIENT_HANDLE_MAGIC 0xd15ea5ed
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Fix variable type declaration after refactoring

[Dan Carpenter]

On Thu, Dec 08, 2016 at 05:07:59PM +0100, Quentin Lambert wrote:
> A recent clean-up declared och_flags as a int rather than fmode_t. This
> lead to the following sparse warning:
> 
> drivers/staging/lustre/lustre/llite/file.c:106:30: warning: restricted
> fmode_t degrades to integer
> 
> This patch fixes this issue.
> 
> Fixes: 1200991234f7 ("staging: lustre: cleanup lustre_lib.h")
   0a1200991234f7

Fixes hash is wrong.  It should start with "0a".

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[Ian Abbott]

On 08/12/16 15:46, SF Markus Elfring wrote:

* Reduce memory allocation sizes for two function calls.


Is this implementation detail worth for further considerations?


I assume you are referring to the allocation of devpriv->ai_urbs and 
devpriv->ao_urbs?  Your patch does not reduce their sizes; `urb` and 
`*devpriv->ai_urbs` have the same type `struct urb *`.  Having said 
that, `sizeof(*devpriv->ai_urbs)` is cleaner code than `sizeof(urb)` in 
this case.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to hyperv.h

[KY Srinivasan]

> -Original Message-
> From: gre...@linuxfoundation.org [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, December 8, 2016 7:57 AM
> To: KY Srinivasan <k...@microsoft.com>
> Cc: kbuild test robot <l...@intel.com>; k...@exchange.microsoft.com;
> o...@aepfle.de; jasow...@redhat.com; Haiyang Zhang
> <haiya...@microsoft.com>; linux-ker...@vger.kernel.org;
> bjorn.helg...@gmail.com; kbuild-...@01.org; a...@canonical.com;
> de...@linuxdriverproject.org; leann.ogasaw...@canonical.com
> Subject: Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to
> hyperv.h
> 
> On Thu, Dec 08, 2016 at 03:26:06PM +, KY Srinivasan wrote:
> >
> >
> > > -Original Message-
> > > From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org]
> On
> > > Behalf Of kbuild test robot
> > > Sent: Thursday, December 8, 2016 1:50 AM
> > > To: k...@exchange.microsoft.com
> > > Cc: o...@aepfle.de; gre...@linuxfoundation.org;
> jasow...@redhat.com;
> > > Haiyang Zhang <haiya...@microsoft.com>; linux-
> ker...@vger.kernel.org;
> > > bjorn.helg...@gmail.com; kbuild-...@01.org; a...@canonical.com;
> > > de...@linuxdriverproject.org; leann.ogasaw...@canonical.com
> > > Subject: Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to
> > > hyperv.h
> > >
> > > Hi Haiyang,
> > >
> > > [auto build test ERROR on next-20161208]
> > > [also build test ERROR on v4.9-rc8]
> > > [cannot apply to linus/master linux/master pci/next v4.9-rc8 v4.9-rc7
> v4.9-
> > > rc6]
> > > [if your patch is applied to the wrong git tree, please drop us a note to
> help
> > > improve the system]
> > >
> > > url:
> > >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> > > .com%2F0day-ci%2Flinux%2Fcommits%2Fkys-exchange-microsoft-
> > > com%2Fhyperv-Move-hv_pci_dev-and-related-structs-to-hyperv-
> > > h%2F20161208-
> > >
> 171643=02%7C01%7Ckys%40microsoft.com%7Ca000870068f645a67492
> > >
> 08d41f4fa963%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6361678
> > >
> 75686353145=2cR2dMEa6sjz94NYUFMx5qC8hEVvXF6uokmCCny%2FgL
> > > E%3D=0
> > > config: x86_64-randconfig-x012-201649 (attached as .config)
> > > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> > > reproduce:
> > > # save the attached .config to linux build tree
> > > make ARCH=x86_64
> > >
> > > All errors (new ones prefixed by >>):
> > >
> > >In file included from drivers/hid/hid-hyperv.c:22:0:
> > > >> include/linux/hyperv.h:1660:25: error: field 'msi_info' has incomplete
> type
> > >  struct msi_domain_info msi_info;
> > > ^~~~
> > >
> > > vim +/msi_info +1660 include/linux/hyperv.h
> > >
> > >   1654struct semaphore enum_sem;
> > >   1655struct list_head resources_for_children;
> > >   1656
> > >   1657struct list_head children;
> > >   1658struct list_head dr_list;
> > >   1659
> > > > 1660struct msi_domain_info msi_info;
> > >   1661struct msi_controller msi_chip;
> > >   1662struct irq_domain *irq_domain;
> > >   1663};
> > >
> >
> >
> > Greg,
> >
> > I sent this patch set to show how we plan to use the API to check if a 
> > device
> is a
> > VMBUS device. Given that multiple trees are involved, the patches were
> based on
> > the linux-next tree. I was planning on submitting these patches with
> minimal
> > inter-tree dependency.
> 
> Ok, but how does this simple patch fail so badly?  Maybe just wait for
> after 4.10-rc1 is out and then try this as everything will be synced up
> then?  It's pretty much past the merge window for all subsystems now
> anyway...

Will do.

Thanks,

K. Y
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: lustre: Fix variable type declaration after refactoring

[Quentin Lambert]

A recent clean-up declared och_flags as a int rather than fmode_t. This
lead to the following sparse warning:

drivers/staging/lustre/lustre/llite/file.c:106:30: warning: restricted
fmode_t degrades to integer

This patch fixes this issue.

Fixes: 1200991234f7 ("staging: lustre: cleanup lustre_lib.h")
Signed-off-by: Quentin Lambert 
---
 drivers/staging/lustre/lustre/include/obd.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/staging/lustre/lustre/include/obd.h
+++ b/drivers/staging/lustre/lustre/include/obd.h
@@ -889,7 +889,7 @@ struct obd_client_handle {
struct md_open_data *och_mod;
struct lustre_handle och_lease_handle; /* open lock for lease */
__u32och_magic;
-   int  och_flags;
+   fmode_t  och_flags;
 };
 
 #define OBD_CLIENT_HANDLE_MAGIC 0xd15ea5ed
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging

[Greg KH]

On Wed, Dec 07, 2016 at 08:19:20PM +, Stuart Yoder wrote:
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Wednesday, December 07, 2016 9:53 AM
> > To: Stuart Yoder 
> > Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; 
> > ag...@suse.de; a...@arndb.de; Leo Li
> > ; Ioana Ciornei ; Catalin 
> > Horghidan
> > ; Laurentiu Tudor ; 
> > Ruxandra Ioana Radulescu
> > 
> > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
> > 
> > On Thu, Dec 01, 2016 at 04:41:26PM -0600, Stuart Yoder wrote:
> > > Move the source files out of staging into their final locations:
> > >   -include files in drivers/staging/fsl-mc/include go to include/linux/fsl
> > >   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
> > >   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
> > >   -README.txt, providing and overview of DPAA goes to
> > >Documentation/dpaa2/overview.txt
> > >   -update MAINTAINERS with new location
> > >
> > > Delete other remaining staging files-- Makefile, Kconfig, TODO
> > 
> > Ok, given that I haven't ever reviewed this code, I had a few questions
> > that I couldn't easily figure out by looking at your code:
> > - what is the lifecycle of your 'struct device' usage?  Who
> >   creates it, who frees it, and who accesses it?
> 
> We embed a 'struct device' inside our bus specific device struct
> 'struct fsl_mc_device'.  So, when a new fsl-mc object is discovered
> on the bus during initial enumeration or hotplug we create a new
> 'struct fsl_mc_device' and do a device_initialize()/device_add().
> (see fsl_mc_device_add() for where this is done)
> 
> 'struct device' is freed when a device is removed-- the reverse
> of the above.

Where is the device freed?  I see you trying to do some "odd" stuff in
fsl_mc_device_remove() by deleting and then putting a device structure.
I can't find a "release()" callback anywhere for your bus, where is it?

What happens when the reference count falls to 0 for your struct device?

> > - root_dprc_count, why are you using an atomic variable for
> >   this?  What is it for other than "look, I'm running!"?
> 
> There can be multiple root buses, and this variable simply tracks the count
> of them.

Why does it matter?

> It's is atomic there might be a theoretical race condition where 2
> buses might be added at the same time.  The root buses are found in
> the device tree and so if there is no chance that device tree
> processing happens in parallel on multiple cores then we could remove
> the atomic.

Why not just use a lock, or better yet, not care about a "count" at all?
I don't see you doing anything with the count, other than emitting a
WARN() if it drops down below 0 for some reason, or when you call
fsl_mc_bus_exists() which for some reason is exported yet no one uses
it...

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to hyperv.h

[gre...@linuxfoundation.org]

On Thu, Dec 08, 2016 at 03:26:06PM +, KY Srinivasan wrote:
> 
> 
> > -Original Message-
> > From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> > Behalf Of kbuild test robot
> > Sent: Thursday, December 8, 2016 1:50 AM
> > To: k...@exchange.microsoft.com
> > Cc: o...@aepfle.de; gre...@linuxfoundation.org; jasow...@redhat.com;
> > Haiyang Zhang <haiya...@microsoft.com>; linux-ker...@vger.kernel.org;
> > bjorn.helg...@gmail.com; kbuild-...@01.org; a...@canonical.com;
> > de...@linuxdriverproject.org; leann.ogasaw...@canonical.com
> > Subject: Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to
> > hyperv.h
> > 
> > Hi Haiyang,
> > 
> > [auto build test ERROR on next-20161208]
> > [also build test ERROR on v4.9-rc8]
> > [cannot apply to linus/master linux/master pci/next v4.9-rc8 v4.9-rc7 v4.9-
> > rc6]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help
> > improve the system]
> > 
> > url:
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> > .com%2F0day-ci%2Flinux%2Fcommits%2Fkys-exchange-microsoft-
> > com%2Fhyperv-Move-hv_pci_dev-and-related-structs-to-hyperv-
> > h%2F20161208-
> > 171643=02%7C01%7Ckys%40microsoft.com%7Ca000870068f645a67492
> > 08d41f4fa963%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6361678
> > 75686353145=2cR2dMEa6sjz94NYUFMx5qC8hEVvXF6uokmCCny%2FgL
> > E%3D=0
> > config: x86_64-randconfig-x012-201649 (attached as .config)
> > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> > reproduce:
> > # save the attached .config to linux build tree
> > make ARCH=x86_64
> > 
> > All errors (new ones prefixed by >>):
> > 
> >In file included from drivers/hid/hid-hyperv.c:22:0:
> > >> include/linux/hyperv.h:1660:25: error: field 'msi_info' has incomplete 
> > >> type
> >  struct msi_domain_info msi_info;
> > ^~~~
> > 
> > vim +/msi_info +1660 include/linux/hyperv.h
> > 
> >   1654  struct semaphore enum_sem;
> >   1655  struct list_head resources_for_children;
> >   1656
> >   1657  struct list_head children;
> >   1658  struct list_head dr_list;
> >   1659
> > > 1660  struct msi_domain_info msi_info;
> >   1661  struct msi_controller msi_chip;
> >   1662  struct irq_domain *irq_domain;
> >   1663  };
> > 
> 
> 
> Greg,
> 
> I sent this patch set to show how we plan to use the API to check if a device 
> is a 
> VMBUS device. Given that multiple trees are involved, the patches were based 
> on
> the linux-next tree. I was planning on submitting these patches with minimal
> inter-tree dependency. 

Ok, but how does this simple patch fail so badly?  Maybe just wait for
after 4.10-rc1 is out and then try this as everything will be synced up
then?  It's pretty much past the merge window for all subsystems now
anyway...

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

[Greg KH]

On Thu, Dec 08, 2016 at 12:33:43AM -0800, k...@exchange.microsoft.com wrote:
> From: Haiyang Zhang 
> 
> We currently use MAC address to match VF and synthetic NICs. Hyper-V
> provides a serial number to both devices for this purpose. This patch
> implements the matching based on VF serial numbers. This is the way
> specified by the protocol and more reliable.
> 
> Signed-off-by: Haiyang Zhang 
> Signed-off-by: K. Y. Srinivasan 
> ---
>  drivers/net/hyperv/netvsc_drv.c |   55 
> ---
>  1 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 9522763..c5778cf 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct net_device 
> *netdev)
>   free_netdev(netdev);
>  }
>  
> -static struct net_device *get_netvsc_bymac(const u8 *mac)
> +static struct net_device *get_netvsc_byvfser(u32 vfser)
>  {
>   struct net_device *dev;
> + struct net_device_context *ndev_ctx;
>  
>   ASSERT_RTNL();
>  
> @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct net_device 
> *netdev)
>   if (dev->netdev_ops != _ops)
>   continue;   /* not a netvsc device */
>  
> - if (ether_addr_equal(mac, dev->perm_addr))
> + ndev_ctx = netdev_priv(dev);
> + if (ndev_ctx->vf_serial == vfser)
>   return dev;
>   }
>  
> @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct net_device 
> *netdev)
>   return NULL;
>  }
>  
> +static u32 netvsc_get_vfser(struct net_device *vf_netdev)
> +{
> + struct device *dev;
> + struct hv_device *hdev;
> + struct hv_pcibus_device *hbus = NULL;
> + struct list_head *iter;
> + struct hv_pci_dev *hpdev;
> + unsigned long flags;
> + u32 vfser = 0;
> + u32 count = 0;
> +
> + for (dev = _netdev->dev; dev; dev = dev->parent) {

You are going to walk the whole device tree backwards?  That's crazy.
And foolish.  And racy and broken (what happens if the tree changes
while you do this?)  Where is the lock being grabbed while this happens?
What about reference counts?  Do you see other drivers ever doing this
(if you do, point them out and I'll go yell at them too...)

> + if (!dev_is_vmbus(dev))
> + continue;

Ick.

Why isn't your parent pointer a vmbus device all the time?  How could
you get burried down in the device hierarchy when you are the driver for
a specific bus type in the first place?  How could this function ever be
called for a device that is NOT of this type?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[SF Markus Elfring]

>> * Reduce memory allocation sizes for two function calls.

Is this implementation detail worth for further considerations?

Regards,
Markus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[SF Markus Elfring]

> Actually, the original code worked fine,

I got my doubts when some memory allocations are attempted without checking
the desired success immediately.


> and these changes will result in an Oops if the allocations fail.  I'll 
> explain why,
> since it isn't obvious without some knowledge of the clean-up strategy used 
> by comedi drivers:

Thanks for your explanation.


> …, and all the other comedi drivers follow the same strategy of leaving 
> clean-up
> to their comedi 'detach' handler.

Are there other source code parts worth for further considerations?

Regards,
Markus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to hyperv.h

[KY Srinivasan]

> -Original Message-
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> Behalf Of kbuild test robot
> Sent: Thursday, December 8, 2016 1:50 AM
> To: k...@exchange.microsoft.com
> Cc: o...@aepfle.de; gre...@linuxfoundation.org; jasow...@redhat.com;
> Haiyang Zhang <haiya...@microsoft.com>; linux-ker...@vger.kernel.org;
> bjorn.helg...@gmail.com; kbuild-...@01.org; a...@canonical.com;
> de...@linuxdriverproject.org; leann.ogasaw...@canonical.com
> Subject: Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to
> hyperv.h
> 
> Hi Haiyang,
> 
> [auto build test ERROR on next-20161208]
> [also build test ERROR on v4.9-rc8]
> [cannot apply to linus/master linux/master pci/next v4.9-rc8 v4.9-rc7 v4.9-
> rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
> 
> url:
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2F0day-ci%2Flinux%2Fcommits%2Fkys-exchange-microsoft-
> com%2Fhyperv-Move-hv_pci_dev-and-related-structs-to-hyperv-
> h%2F20161208-
> 171643=02%7C01%7Ckys%40microsoft.com%7Ca000870068f645a67492
> 08d41f4fa963%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6361678
> 75686353145=2cR2dMEa6sjz94NYUFMx5qC8hEVvXF6uokmCCny%2FgL
> E%3D=0
> config: x86_64-randconfig-x012-201649 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
> 
> All errors (new ones prefixed by >>):
> 
>In file included from drivers/hid/hid-hyperv.c:22:0:
> >> include/linux/hyperv.h:1660:25: error: field 'msi_info' has incomplete type
>  struct msi_domain_info msi_info;
> ^~~~
> 
> vim +/msi_info +1660 include/linux/hyperv.h
> 
>   1654struct semaphore enum_sem;
>   1655struct list_head resources_for_children;
>   1656
>   1657struct list_head children;
>   1658struct list_head dr_list;
>   1659
> > 1660struct msi_domain_info msi_info;
>   1661struct msi_controller msi_chip;
>   1662struct irq_domain *irq_domain;
>   1663};
> 


Greg,

I sent this patch set to show how we plan to use the API to check if a device 
is a 
VMBUS device. Given that multiple trees are involved, the patches were based on
the linux-next tree. I was planning on submitting these patches with minimal
inter-tree dependency. 

Regards,

K. Y
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01
> .org%2Fpipermail%2Fkbuild-
> all=02%7C01%7Ckys%40microsoft.com%7Ca000870068f645a6749208d4
> 1f4fa963%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63616787568
> 6353145=5T776prLOj7qzsQa%2B3Giit1UFvZJ6D%2FdL0I8k1qKfIg%3D
> eserved=0   Intel Corporation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging-COMEDI: Fine-tuning for three functions

[SF Markus Elfring]

> You do realize that I no longer take patches from you for any of the
> subsystems I maintain, right?

Not so far.

It seems that you would like to present new information for our
challenging collaboration.


> This patch series is one reason why...

I hope that corresponding disagreements around shown change possibilities
can still be clarified somehow.
Would you like to check further improvements for the affected
source code here?

Regards,
Markus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: comedi: comedidev.h: Document usage of 'detach' handler

[Ian Abbott]

Document when the "detach" handler function pointed to by the `detach`
member of a `struct comedi_driver` is called by the comedi core, and how
the "attach" and "auto_attach" handlers can defer clean-up to it when
they return an error to the comedi core.  This is already mentioned as
part of the documentation for `comedi_auto_config()`, but is useful to
document it for `struct comedi_driver` as well, since
`comedi_auto_config()` is not usually called directly by low-level
comedi drivers, and it is not called at all for "legacy" comedi devices
that are configured manually.

Signed-off-by: Ian Abbott 
---
 drivers/staging/comedi/comedidev.h | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index dcb6376..0c7c37a 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -426,6 +426,18 @@ enum comedi_cb {
  * handler will be called with the COMEDI device structure's board_ptr member
  * pointing to the matched pointer to a board name within the driver's private
  * array of static, read-only board type information.
+ *
+ * The @detach handler has two roles.  If a COMEDI device was successfully
+ * configured by the @attach or @auto_attach handler, it is called when the
+ * device is being deconfigured (by the %COMEDI_DEVCONFIG ioctl, or due to
+ * unloading of the driver, or due to device removal).  It is also called when
+ * the @attach or @auto_attach handler returns an error.  Therefore, the
+ * @attach or @auto_attach handlers can defer clean-up on error until the
+ * @detach handler is called.  If the @attach or @auto_attach handlers free
+ * any resources themselves, they must prevent the @detach handler from
+ * freeing the same resources.  The @detach handler must not assume that all
+ * resources requested by the @attach or @auto_attach handler were
+ * successfully allocated.
  */
 struct comedi_driver {
/* private: */
-- 
2.10.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/5] staging-COMEDI: Fine-tuning for three functions

[Greg Kroah-Hartman]

On Thu, Dec 08, 2016 at 12:30:20PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 8 Dec 2016 11:37:37 +0100
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   Combine four kcalloc() calls into one in serial2002_setup_subdevs()
>   Split a condition check in usbdux_alloc_usb_buffers()
>   Move an assignment in usbdux_alloc_usb_buffers()
>   Split a condition check in usbduxsigma_alloc_usb_buffers()
>   Move an assignment in usbduxsigma_alloc_usb_buffers()
> 
>  drivers/staging/comedi/drivers/serial2002.c  | 22 +-
>  drivers/staging/comedi/drivers/usbdux.c  | 56 ++-
>  drivers/staging/comedi/drivers/usbduxsigma.c | 66 
> ++--
>  3 files changed, 108 insertions(+), 36 deletions(-)

You do realize that I no longer take patches from you for any of the
subsystems I maintain, right?  This patch series is one reason why...

I suggest working on other projects to learn C better first.

best of luck,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[Ian Abbott]

On 08/12/16 11:37, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Thu, 8 Dec 2016 11:15:40 +0100

The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbduxsigma_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.

Allocated memory was also not released if one of these function
calls failed.

* Reduce memory allocation sizes for two function calls.

* Split a condition check for memory allocation failures.

* Add more exception handling.

Fixes: 65989c030bbca96be45ed137f6384dbd46030d10 ("staging: comedi: usbduxsigma: 
factor usb buffer allocation out of (*probe)")

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 61 ++--
 1 file changed, 49 insertions(+), 12 deletions(-)


My comments on PATCH 2/5 about how comedi drivers handle clean-up apply 
to this patch too.  So this patch will cause an Oops in the unlikely 
event of running out of memory during buffer allocation.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_subdevs()

[Dan Carpenter]

The original code was simpler.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[Ian Abbott]

On 08/12/16 11:34, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Thu, 8 Dec 2016 10:01:54 +0100

The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbdux_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.

Allocated memory was also not released if one of these function
calls failed.

* Split a condition check for memory allocation failures.

* Add more exception handling.

Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up 
usbdux_alloc_usb_buffers()")

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/usbdux.c | 53 ++---
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbdux.c 
b/drivers/staging/comedi/drivers/usbdux.c
index f4f05d29d30d..d7d683bd669c 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct 
comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
-   int i;
+   int i, x;

devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+   if (!devpriv->dux_commands)
+   return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+   if (!devpriv->in_buf)
+   goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
+   if (!devpriv->insn_buf)
+   goto free_in_buf;
+
devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
   GFP_KERNEL);
+   if (!devpriv->ai_urbs)
+   goto free_insn_buf;
+
devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
   GFP_KERNEL);
-   if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
-   !devpriv->ai_urbs || !devpriv->ao_urbs)
-   return -ENOMEM;
+   if (!devpriv->ao_urbs)
+   goto free_ai_urbs;

for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;

urb->dev = usb;
@@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_n_ai_urbs;

urb->complete = usbduxsub_ai_isoc_irq;
urb->number_of_packets = 1;
@@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;

urb->dev = usb;
@@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_n_ao_urbs;

urb->complete = usbduxsub_ao_isoc_irq;
urb->number_of_packets = 1;
@@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct 
comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
devpriv->pwm_urb = urb;

/* max bulk ep size in high speed */
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
   GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_pwm_urb;
}

return 0;
+free_pwm_urb:
+   usb_free_urb(urb);
+free_n_ao_urbs:
+   for (x = 0; x < i; ++x) {
+   kfree(devpriv->ao_urbs[x]->transfer_buffer);
+   usb_free_urb(devpriv->ao_urbs[x]);
+   }
+free_n_ai_urbs:
+   for (x = 0; x < i; ++x) {
+   kfree(devpriv->ai_urbs[x]->transfer_buffer);
+   usb_free_urb(devpriv->ai_urbs[x]);
+   }
+   kfree(devpriv->ao_urbs);
+free_ai_urbs:
+   kfree(devpriv->ai_urbs);
+free_insn_buf:
+   kfree(devpriv->insn_buf);

Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[Dan Carpenter]

On Thu, Dec 08, 2016 at 12:34:27PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 8 Dec 2016 10:01:54 +0100
> 
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbdux_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
> 
> Allocated memory was also not released if one of these function
> calls failed.
> 
> * Split a condition check for memory allocation failures.
> 
> * Add more exception handling.
> 
> Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: 
> tidy up usbdux_alloc_usb_buffers()")
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/staging/comedi/drivers/usbdux.c | 53 
> ++---
>  1 file changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbdux.c 
> b/drivers/staging/comedi/drivers/usbdux.c
> index f4f05d29d30d..d7d683bd669c 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct 
> comedi_device *dev)
>   struct usb_device *usb = comedi_to_usb_dev(dev);
>   struct usbdux_private *devpriv = dev->private;
>   struct urb *urb;
> - int i;
> + int i, x;
>  
>   devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
> + if (!devpriv->dux_commands)
> + return -ENOMEM;
> +
>   devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
> + if (!devpriv->in_buf)
> + goto free_commands;
> +
>   devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
> + if (!devpriv->insn_buf)
> + goto free_in_buf;
> +
>   devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
>  GFP_KERNEL);
> + if (!devpriv->ai_urbs)
> + goto free_insn_buf;
> +
>   devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
>  GFP_KERNEL);
> - if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
> - !devpriv->ai_urbs || !devpriv->ao_urbs)
> - return -ENOMEM;
> + if (!devpriv->ao_urbs)
> + goto free_ai_urbs;
>  
>   for (i = 0; i < devpriv->n_ai_urbs; i++) {
>   /* one frame: 1ms */
>   urb = usb_alloc_urb(1, GFP_KERNEL);
>   if (!urb)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
>   devpriv->ai_urbs[i] = urb;
>  
>   urb->dev = usb;
> @@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct 
> comedi_device *dev)
>   urb->transfer_flags = URB_ISO_ASAP;
>   urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
>   if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
>  
>   urb->complete = usbduxsub_ai_isoc_irq;
>   urb->number_of_packets = 1;
> @@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct 
> comedi_device *dev)
>   /* one frame: 1ms */
>   urb = usb_alloc_urb(1, GFP_KERNEL);
>   if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>   devpriv->ao_urbs[i] = urb;
>  
>   urb->dev = usb;
> @@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct 
> comedi_device *dev)
>   urb->transfer_flags = URB_ISO_ASAP;
>   urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
>   if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>  
>   urb->complete = usbduxsub_ao_isoc_irq;
>   urb->number_of_packets = 1;
> @@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct 
> comedi_device *dev)
>   if (devpriv->pwm_buf_sz) {
>   urb = usb_alloc_urb(0, GFP_KERNEL);
>   if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>   devpriv->pwm_urb = urb;
>  
>   /* max bulk ep size in high speed */
>   urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
>  GFP_KERNEL);
>   if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_pwm_urb;
>   }
>  
>   return 0;
> +free_pwm_urb:
> + usb_free_urb(urb);
> +free_n_ao_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ao_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ao_urbs[x]);
> + }
> +free_n_ai_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ai_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ai_urbs[x]);
> 

Re: [PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()

[Dan Carpenter]

This one is pointless.  It's just a style issue that you invented.  Only
Joe Perches is allowed to invent new style guidelines.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[Dan Carpenter]

Same bug.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Dan Carpenter]

On Thu, Dec 08, 2016 at 02:50:49PM +0300, Dan Carpenter wrote:
> On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote:
> > But it would make me very happy if someone would add a similar grouping
> > functionality to dyndbg to make it easy to enable a set of debug
> > messages in a driver.
> 
> Thats seems like a reasonable thing as well.

I actually like the ath code...  We could easily change it to be more
generic and make it a top level function for everyone to use.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Dan Carpenter]

I don't have a problem with the ath debug printks.  Larry asked me for
examples of better debug functions and the ath code is an example.
Literally, any existing debug functions are better than the
BTC_TRACE_STRING() stuff.

On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote:
> But it would make me very happy if someone would add a similar grouping
> functionality to dyndbg to make it easy to enable a set of debug
> messages in a driver.

Thats seems like a reasonable thing as well.

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_subdevs()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 8 Dec 2016 07:37:29 +0100

The function "kcalloc" was called in three cases by the function
"serial2002_setup_subdevs" without checking immediately if it failed.
This issue was detected by using the Coccinelle software.

* Perform the desired memory allocation (and release at the end) by a single
  function call instead.

* Adjust a jump target so that a redundant check is avoided.

Fixes: 623a73926c7012e3bb132e225621890207f5c611 ("staging: comedi: serial2002: 
split up serial_2002_open()")

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/serial2002.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/comedi/drivers/serial2002.c 
b/drivers/staging/comedi/drivers/serial2002.c
index 0d33e520f635..9542f4f8afe0 100644
--- a/drivers/staging/comedi/drivers/serial2002.c
+++ b/drivers/staging/comedi/drivers/serial2002.c
@@ -392,6 +392,7 @@ static int serial2002_setup_subdevice(struct 
comedi_subdevice *s,
 static int serial2002_setup_subdevs(struct comedi_device *dev)
 {
struct serial2002_private *devpriv = dev->private;
+   struct config_t *array;
struct config_t *di_cfg;
struct config_t *do_cfg;
struct config_t *ai_cfg;
@@ -402,15 +403,17 @@ static int serial2002_setup_subdevs(struct comedi_device 
*dev)
int i;
 
/* Allocate the temporary structs to hold the configuration data */
-   di_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
-   do_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
-   ai_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
-   ao_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
-   if (!di_cfg || !do_cfg || !ai_cfg || !ao_cfg) {
+   array = kcalloc(4 * 32, sizeof(*cfg), GFP_KERNEL);
+   if (!array) {
result = -ENOMEM;
-   goto err_alloc_configs;
+   goto check_tty;
}
 
+   di_cfg = array;
+   do_cfg = array + 1 * 32;
+   ai_cfg = array + 2 * 32;
+   ao_cfg = array + 3 * 32;
+
/* Read the configuration from the connected device */
serial2002_tty_setspeed(devpriv->tty, devpriv->speed);
serial2002_poll_channel(devpriv->tty, 31);
@@ -534,13 +537,10 @@ static int serial2002_setup_subdevs(struct comedi_device 
*dev)
}
}
 
-err_alloc_configs:
-   kfree(di_cfg);
-   kfree(do_cfg);
-   kfree(ai_cfg);
-   kfree(ao_cfg);
+   kfree(array);
 
if (result) {
+check_tty:
if (devpriv->tty) {
filp_close(devpriv->tty, NULL);
devpriv->tty = NULL;
-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/5] staging: comedi: usbduxsigma: Move an assignment in usbduxsigma_alloc_usb_buffers()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 8 Dec 2016 11:20:38 +0100

Move one assignment for the local variable "usb" so that its setting
will only be performed after some memory allocations succeeded
by this function.

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c 
b/drivers/staging/comedi/drivers/usbduxsigma.c
index 8c04aa5339f3..7c1f9198447a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1338,7 +1338,7 @@ static int usbduxsigma_firmware_upload(struct 
comedi_device *dev,
 
 static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
 {
-   struct usb_device *usb = comedi_to_usb_dev(dev);
+   struct usb_device *usb;
struct usbduxsigma_private *devpriv = dev->private;
struct urb *urb;
int i, x;
@@ -1367,6 +1367,7 @@ static int usbduxsigma_alloc_usb_buffers(struct 
comedi_device *dev)
if (!devpriv->ao_urbs)
goto free_ai_urbs;
 
+   usb = comedi_to_usb_dev(dev);
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Kalle Valo]

Dan Carpenter  writes:

> On Wed, Dec 07, 2016 at 02:16:06PM +0200, Kalle Valo wrote:
>> We have disccused this before, but for wireless it's not really that
>> simple. AFAIK with dyndbg you can only control the messages per line
>> (painful to enable group of messages) or per file (enables too many
>> messages). In wireless we have cases when we need to enable group of
>> messages, but not all.
>
> You can turn them on by the function or a range of lines, then disable
> the spammy lines.  With these new debug macros you can't do that so this
> is a step backwards.

>From my point of view dyndbg is step backwards. Line numbers change all
the time and you can't have simple instructions like this for users and
developers working with the driver:

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/debug#debug_log_messages

I don't want waste time with this anymore as we seem to just disagree.
My point here was that in wireless we have used log wrappers before and
will continue to use them in certain drivers as dyndbg is not enough.
But it would make me very happy if someone would add a similar grouping
functionality to dyndbg to make it easy to enable a set of debug
messages in a driver.

-- 
Kalle Valo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 8 Dec 2016 11:15:40 +0100

The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbduxsigma_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.

Allocated memory was also not released if one of these function
calls failed.

* Reduce memory allocation sizes for two function calls.

* Split a condition check for memory allocation failures.

* Add more exception handling.

Fixes: 65989c030bbca96be45ed137f6384dbd46030d10 ("staging: comedi: usbduxsigma: 
factor usb buffer allocation out of (*probe)")

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 61 ++--
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c 
b/drivers/staging/comedi/drivers/usbduxsigma.c
index 456e9f13becb..8c04aa5339f3 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1341,22 +1341,37 @@ static int usbduxsigma_alloc_usb_buffers(struct 
comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbduxsigma_private *devpriv = dev->private;
struct urb *urb;
-   int i;
+   int i, x;
 
devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+   if (!devpriv->dux_commands)
+   return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+   if (!devpriv->in_buf)
+   goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
-   devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(urb), GFP_KERNEL);
-   devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(urb), GFP_KERNEL);
-   if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
-   !devpriv->ai_urbs || !devpriv->ao_urbs)
-   return -ENOMEM;
+   if (!devpriv->insn_buf)
+   goto free_in_buf;
+
+   devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs,
+  sizeof(*devpriv->ai_urbs),
+  GFP_KERNEL);
+   if (!devpriv->ai_urbs)
+   goto free_insn_buf;
+
+   devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs,
+  sizeof(*devpriv->ao_urbs),
+  GFP_KERNEL);
+   if (!devpriv->ao_urbs)
+   goto free_ai_urbs;
 
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;
urb->dev = usb;
/* will be filled later with a pointer to the comedi-device */
@@ -1366,7 +1381,7 @@ static int usbduxsigma_alloc_usb_buffers(struct 
comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_n_ai_urbs;
urb->complete = usbduxsigma_ai_urb_complete;
urb->number_of_packets = 1;
urb->transfer_buffer_length = SIZEINBUF;
@@ -1378,7 +1393,7 @@ static int usbduxsigma_alloc_usb_buffers(struct 
comedi_device *dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;
urb->dev = usb;
/* will be filled later with a pointer to the comedi-device */
@@ -1388,7 +1403,7 @@ static int usbduxsigma_alloc_usb_buffers(struct 
comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
urb->complete = usbduxsigma_ao_urb_complete;
urb->number_of_packets = 1;
urb->transfer_buffer_length = SIZEOUTBUF;
@@ -1400,16 +1415,38 @@ static int usbduxsigma_alloc_usb_buffers(struct 
comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
devpriv->pwm_urb = urb;
 
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
   GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   

[PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 8 Dec 2016 10:13:56 +0100

Move one assignment for the local variable "usb" so that its setting
will only be performed after some memory allocations succeeded
by this function.

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/usbdux.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/usbdux.c 
b/drivers/staging/comedi/drivers/usbdux.c
index d7d683bd669c..7efeac71161b 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1446,7 +1446,7 @@ static int usbdux_firmware_upload(struct comedi_device 
*dev,
 
 static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
 {
-   struct usb_device *usb = comedi_to_usb_dev(dev);
+   struct usb_device *usb;
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
int i, x;
@@ -1473,6 +1473,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
if (!devpriv->ao_urbs)
goto free_ai_urbs;
 
+   usb = comedi_to_usb_dev(dev);
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 8 Dec 2016 10:01:54 +0100

The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbdux_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.

Allocated memory was also not released if one of these function
calls failed.

* Split a condition check for memory allocation failures.

* Add more exception handling.

Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy 
up usbdux_alloc_usb_buffers()")

Signed-off-by: Markus Elfring 
---
 drivers/staging/comedi/drivers/usbdux.c | 53 ++---
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbdux.c 
b/drivers/staging/comedi/drivers/usbdux.c
index f4f05d29d30d..d7d683bd669c 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct 
comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
-   int i;
+   int i, x;
 
devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+   if (!devpriv->dux_commands)
+   return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+   if (!devpriv->in_buf)
+   goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
+   if (!devpriv->insn_buf)
+   goto free_in_buf;
+
devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
   GFP_KERNEL);
+   if (!devpriv->ai_urbs)
+   goto free_insn_buf;
+
devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
   GFP_KERNEL);
-   if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
-   !devpriv->ai_urbs || !devpriv->ao_urbs)
-   return -ENOMEM;
+   if (!devpriv->ao_urbs)
+   goto free_ai_urbs;
 
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;
 
urb->dev = usb;
@@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_n_ai_urbs;
 
urb->complete = usbduxsub_ai_isoc_irq;
urb->number_of_packets = 1;
@@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;
 
urb->dev = usb;
@@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device 
*dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
 
urb->complete = usbduxsub_ao_isoc_irq;
urb->number_of_packets = 1;
@@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct 
comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
-   return -ENOMEM;
+   goto free_n_ao_urbs;
devpriv->pwm_urb = urb;
 
/* max bulk ep size in high speed */
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
   GFP_KERNEL);
if (!urb->transfer_buffer)
-   return -ENOMEM;
+   goto free_pwm_urb;
}
 
return 0;
+free_pwm_urb:
+   usb_free_urb(urb);
+free_n_ao_urbs:
+   for (x = 0; x < i; ++x) {
+   kfree(devpriv->ao_urbs[x]->transfer_buffer);
+   usb_free_urb(devpriv->ao_urbs[x]);
+   }
+free_n_ai_urbs:
+   for (x = 0; x < i; ++x) {
+   kfree(devpriv->ai_urbs[x]->transfer_buffer);
+   usb_free_urb(devpriv->ai_urbs[x]);
+   }
+   kfree(devpriv->ao_urbs);
+free_ai_urbs:
+   kfree(devpriv->ai_urbs);
+free_insn_buf:
+   kfree(devpriv->insn_buf);
+free_in_buf:
+   

[PATCH 0/5] staging-COMEDI: Fine-tuning for three functions

[SF Markus Elfring]

From: Markus Elfring 
Date: Thu, 8 Dec 2016 11:37:37 +0100

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Combine four kcalloc() calls into one in serial2002_setup_subdevs()
  Split a condition check in usbdux_alloc_usb_buffers()
  Move an assignment in usbdux_alloc_usb_buffers()
  Split a condition check in usbduxsigma_alloc_usb_buffers()
  Move an assignment in usbduxsigma_alloc_usb_buffers()

 drivers/staging/comedi/drivers/serial2002.c  | 22 +-
 drivers/staging/comedi/drivers/usbdux.c  | 56 ++-
 drivers/staging/comedi/drivers/usbduxsigma.c | 66 ++--
 3 files changed, 108 insertions(+), 36 deletions(-)

-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to hyperv.h

[kbuild test robot]

Hi Haiyang,

[auto build test ERROR on next-20161208]
[also build test ERROR on v4.9-rc8]
[cannot apply to linus/master linux/master pci/next v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/kys-exchange-microsoft-com/hyperv-Move-hv_pci_dev-and-related-structs-to-hyperv-h/20161208-171643
config: x86_64-allyesdebian (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from drivers/input/serio/hyperv-keyboard.c:18:0:
>> include/linux/hyperv.h:1654:19: error: field 'enum_sem' has incomplete type
 struct semaphore enum_sem;
  ^~~~

vim +/enum_sem +1654 include/linux/hyperv.h

  1648  struct completion remove_event;
  1649  struct pci_bus *pci_bus;
  1650  spinlock_t config_lock; /* Avoid two threads writing index page 
*/
  1651  spinlock_t device_list_lock;/* Protect lists below */
  1652  void __iomem *cfg_addr;
  1653  
> 1654  struct semaphore enum_sem;
  1655  struct list_head resources_for_children;
  1656  
  1657  struct list_head children;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] hyperv: Move hv_pci_dev and related structs to hyperv.h

[kbuild test robot]

Hi Haiyang,

[auto build test ERROR on next-20161208]
[also build test ERROR on v4.9-rc8]
[cannot apply to linus/master linux/master pci/next v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/kys-exchange-microsoft-com/hyperv-Move-hv_pci_dev-and-related-structs-to-hyperv-h/20161208-171643
config: x86_64-randconfig-x012-201649 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from drivers/hid/hid-hyperv.c:22:0:
>> include/linux/hyperv.h:1660:25: error: field 'msi_info' has incomplete type
 struct msi_domain_info msi_info;
^~~~

vim +/msi_info +1660 include/linux/hyperv.h

  1654  struct semaphore enum_sem;
  1655  struct list_head resources_for_children;
  1656  
  1657  struct list_head children;
  1658  struct list_head dr_list;
  1659  
> 1660  struct msi_domain_info msi_info;
  1661  struct msi_controller msi_chip;
  1662  struct irq_domain *irq_domain;
  1663  };

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel