Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
On Sat, Apr 27, 2019 at 02:17:42AM -0400, Sven Van Asbroeck wrote: > Hello Nicholas, thank you for your contribution, I really appreciate it ! > See inline comments below. > > On Sat, Apr 27, 2019 at 12:32 AM Nicholas Mc Guire wrote: > > > > wait_for_completion_timeout() returns unsigned long (0 on timeout or > > remaining jiffies) not int. > > Nice catch ! > > > thus there is no negative case to check for > > here. > > The current code can only break if wait_for_completion_timeout() > returns an unsigned long large enough to make the "int ret" turn > negative. This could result in probe() returning a nonsensical error > value, even though the wait did not time out. > > Fortunately that currently cannot happen, because TIMEOUT > (2*HZ) won't overflow an int. ok - thats benign then - never the less since code tends to get copied it would be good to make it comply with API spec > > That being said, this return value type mismatch should definitely > be fixed up. > > > > > Signed-off-by: Nicholas Mc Guire > > --- > > > > Problem located with experimental API conformance checking cocci script > > > > Q: It is not really clear if the proposed fix is the right thing here or if > >this should not be using wait_for_completion_interruptible - which would > >return -ERESTARTSYS on interruption. Someone that knows the details of > >this driver would need to check what condition should initiate the > >goto err_reset; which was actually unreachable in the current code. > > It's used in probe(), so no need for an interruptible wait, AFAIK. > It can stay as-is. > > > > > Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m, > > HMS_ANYBUSS_BUS=m > > (some unrelated sparse warnings (cast to restricted __be16)) > > That sounds interesting too. Could you provide more details? make C=1 drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16 drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16 drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16 drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16 drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted > > > > > - ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT); > > - if (ret == 0) > > + time_left = wait_for_completion_timeout(&cd->card_boot, TIMEOUT); > > + if (time_left == 0) > > ret = -ETIMEDOUT; > > - if (ret < 0) > > - goto err_reset; > > NAK. This does not jump to err_reset on timeout. > > May I suggest the following instead ? (manual formatting) > > - ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT); > - if (ret == 0) > - ret = -ETIMEDOUT; > - if (ret < 0) > - goto err_reset; > + if (!wait_for_completion_timeout(&cd->card_boot, TIMEOUT)) { > + ret = -ETIMEDOUT; > + goto err_reset; > + } Ah - ok - that was the bit missing for me ! how that goto branch would be reached or if it should be dropped as it had not been reachable in the past. I'll send the V2 later today then. thx! hofrat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
On Sat, Apr 27, 2019 at 3:01 AM Nicholas Mc Guire wrote: > > > (some unrelated sparse warnings (cast to restricted __be16)) > > > > That sounds interesting too. Could you provide more details? > > make C=1 > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted > __be16 > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted > __be16 > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted > __be16 > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted > __be16 > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type, sizeof(fieldbus_type)); fieldbus_type = be16_to_cpu(fieldbus_type); Probably because the parameter to be16_to_cpu() should be __be16. Would you like to spin a separate patch for this too? Or shall I? > I'll send the V2 later today then. Thank you ! ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH RFC V2] staging: kpc2000: use int for wait_for_completion_interruptible
weit_for_completion_interruptible returns in (0 on completion and -ERESTARTSYS on interruption) - so use an int not long for API conformance and simplify the logic here a bit: need not check explicitly for == 0 as this is either -ERESTARTSYS or 0. Signed-off-by: Nicholas Mc Guire --- Problem located with experimental API conformance checking cocci script V2: kbuild reported a missing closing comment - seems that I managed send out the the initial version before compile testing/checkpatching sorry for the noise. Not sure if making such point-wise fixes makes much sense - this driver has a number of issues both style-wise and API compliance wise. Note that kpc_dma_transfer() returns int not long - currently rv (long) is being returned in most places (some places do return int) - so the return handling here is a bit inconsistent. Patch was compile-tested with: x86_64_defconfig + KPC2000=y, KPC2000_DMA=y (with a number of unrelated sparse warnings about non-declared symbols, and smatch warnings about overflowing constants as well as coccicheck warning about usless casting) Patch is against 5.1-rc6 (localversion-next is next-20190426) drivers/staging/kpc2000/kpc_dma/fileops.c | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c index 5741d2b..66f0d5a 100644 --- a/drivers/staging/kpc2000/kpc_dma/fileops.c +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c @@ -38,6 +38,7 @@ int kpc_dma_transfer(struct dev_private_data *priv, struct kiocb *kcb, unsigned { unsigned int i = 0; long rv = 0; + int ret = 0; struct kpc_dma_device *ldev; struct aio_cb_data *acd; DECLARE_COMPLETION_ONSTACK(done); @@ -180,16 +181,17 @@ int kpc_dma_transfer(struct dev_private_data *priv, struct kiocb *kcb, unsigned // If this is a synchronous kiocb, we need to put the calling process to sleep until the transfer is complete if (kcb == NULL || is_sync_kiocb(kcb)){ - rv = wait_for_completion_interruptible(&done); - // If the user aborted (rv == -ERESTARTSYS), we're no longer responsible for cleaning up the acd - if (rv == -ERESTARTSYS){ + ret = wait_for_completion_interruptible(&done); + /* If the user aborted (ret == -ERESTARTSYS), we're +* no longer responsible for cleaning up the acd +*/ + if (ret) { acd->cpl = NULL; - } - if (rv == 0){ - rv = acd->len; + } else { + ret = acd->len; kfree(acd); } - return rv; + return ret; } return -EIOCBQUEUED; -- 2.1.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
On Sat, Apr 27, 2019 at 03:20:54AM -0400, Sven Van Asbroeck wrote: > On Sat, Apr 27, 2019 at 3:01 AM Nicholas Mc Guire wrote: > > > > (some unrelated sparse warnings (cast to restricted __be16)) > > > > > > That sounds interesting too. Could you provide more details? > > > > make C=1 > > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to > > restricted __be16 > > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to > > restricted __be16 > > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to > > restricted __be16 > > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to > > restricted __be16 > > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted > > regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type, i> sizeof(fieldbus_type)); > fieldbus_type = be16_to_cpu(fieldbus_type); > > Probably because the parameter to be16_to_cpu() should be __be16. > Would you like to spin a separate patch for this too? Or shall I? so the issue is simply that the endiannes anotatoin is missing even though the conversion is being done - with other words there is no code lvel funcitonal bug here but rather sparse needs the anotation to verify correctness and that is missing. Just want to make sure I understand this before I try to "fix" a sparse warning. thx! hofrat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API
On Fri, Apr 26, 2019 at 10:41:20AM +0100, Ian Abbott wrote: > On 25/04/2019 18:13, Greg Kroah-Hartman wrote: > > On Thu, Apr 25, 2019 at 05:26:44PM +0100, Ian Abbott wrote: > > > The "comedi_isadma" module calls `dma_alloc_coherent()` and > > > `dma_free_coherent()` with a NULL device pointer which is no longer > > > allowed. If the `hw_dev` member of the `struct comedi_device` has been > > > set to a valid device, that can be used instead. Unfortunately, all the > > > current users of the "comedi_isadma" module leave the `hw_dev` member > > > set to NULL. In that case, use a static dummy fallback device structure > > > with the coherent DMA mask set to the ISA bus limit of 16MB. > > > > > > Signed-off-by: Ian Abbott > > > --- > > > drivers/staging/comedi/drivers/comedi_isadma.c | 15 +-- > > > drivers/staging/comedi/drivers/comedi_isadma.h | 3 +++ > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c > > > b/drivers/staging/comedi/drivers/comedi_isadma.c > > > index b77dc8d5d3ff..8929952516a1 100644 > > > --- a/drivers/staging/comedi/drivers/comedi_isadma.c > > > +++ b/drivers/staging/comedi/drivers/comedi_isadma.c > > > @@ -14,6 +14,16 @@ > > > #include "comedi_isadma.h" > > > +/* > > > + * Fallback device used when hardware device is NULL. > > > + * This can be removed after drivers have been converted to use > > > isa_driver. > > > + */ > > > +static struct device fallback_dev = { > > > + .init_name = "comedi_isadma fallback device", > > > + .coherent_dma_mask = DMA_BIT_MASK(24), > > > + .dma_mask = &fallback_dev.coherent_dma_mask, > > > +}; > > > > Ick, no, static struct device are a very bad idea as this is a reference > > counted structure and making it static can cause odd problems. > > This was based on the use of `struct device x86_dma_fallback_dev` in > "arch/x86/kernel/pci-dma.c", and `static struct device isa_dma_dev` in > "arch/arm/kernel/dma-isa.c", but perhaps it is not appropriate in non-arch > code. No, those are probably broken as well :) Ah, I see why, ugh, ISA. > > Why not just create a "real" one? Or better yet, use the real device > > for the comedi device as all of these drivers should have one now. > > I suppose I could use the comedi class device pointed to by the `class_dev` > member of `struct comedi_device` (although that could also be NULL because > the comedi core does not currently treat `device_create()` failures as > fatal). Why would device_create() fail? If it does, the driver should not have been bound to anything. And no, this shouldn't be the class device, but the "real" hardware device that does dma, which is what is needed anyway here to allow the DMA to happen properly. I guess the problem is with the ISA drivers, right? And I doubt that is ever going to get fixed up, hopefully just deleted :) Your second patch is "good enough", I'll go queue that up now. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
On Sat, Apr 27, 2019 at 7:18 AM Nicholas Mc Guire wrote: > > so the issue is simply that the endiannes anotatoin is missing even > though the conversion is being done - with other words there is no code > lvel funcitonal bug here but rather sparse needs the anotation to verify > correctness and that is missing. Just want to make sure I understand > this before I try to "fix" a sparse warning. Correct. Thanks, Sven ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vc04_services: bcm2835-camera: Modify return statement.
Modify return statement and remove the respective assignment. Issue found by Coccinelle. Signed-off-by: Vatsala Narang --- drivers/staging/vc04_services/bcm2835-camera/controls.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c index 2e0a422cdf3e..9841c30450ce 100644 --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c @@ -270,11 +270,9 @@ static int ctrl_set_rotate(struct bm2835_mmal_dev *dev, if (ret < 0) return ret; - ret = vchiq_mmal_port_parameter_set(dev->instance, &camera->output[2], + return vchiq_mmal_port_parameter_set(dev->instance, &camera->output[2], mmal_ctrl->mmal_id, &u32_value, sizeof(u32_value)); - - return ret; } static int ctrl_set_flip(struct bm2835_mmal_dev *dev, @@ -313,11 +311,9 @@ static int ctrl_set_flip(struct bm2835_mmal_dev *dev, if (ret < 0) return ret; - ret = vchiq_mmal_port_parameter_set(dev->instance, &camera->output[2], + return vchiq_mmal_port_parameter_set(dev->instance, &camera->output[2], mmal_ctrl->mmal_id, &u32_value, sizeof(u32_value)); - - return ret; } static int ctrl_set_exposure(struct bm2835_mmal_dev *dev, -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: sm750fb: Change *array into *const array
Resolve checkpatch warning for static const char * array by using const pointers. Checkpatch Warning in sm750.c: static const char * array should probably be static const char * const Signed-off-by: Kelsey Skunberg --- 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 105089b97bf5..59568d18ce23 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -748,7 +748,7 @@ static int lynxfb_set_fbinfo(struct fb_info *info, int index) lynx750_ext, NULL, vesa_modes, }; int cdb[] = {ARRAY_SIZE(lynx750_ext), 0, VESA_MODEDB_SIZE}; - static const char *mdb_desc[] = { + static const char * const mdb_desc[] = { "driver prepared modes", "kernel prepared default modedb", "kernel HELPERS prepared vesa_modes", -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: kpc2000_spi: eliminated duplicate initialization of master local variable.
master was being initialized to a particular value and then having the same value assigned to it immediately afterwards. Removed the initializer. Since the value assigned to master was dynamically allocated, this fixes a memory-leak. Signed-off-by: Jeremy Sowden --- drivers/staging/kpc2000/kpc_spi/spi_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/kpc2000/kpc_spi/spi_driver.c b/drivers/staging/kpc2000/kpc_spi/spi_driver.c index e77f04bf02d9..c0999e080577 100644 --- a/drivers/staging/kpc2000/kpc_spi/spi_driver.c +++ b/drivers/staging/kpc2000/kpc_spi/spi_driver.c @@ -408,7 +408,7 @@ static int kp_spi_probe(struct platform_device *pldev) { struct kpc_core_device_platdata *drvdata; -struct spi_master *master = spi_alloc_master(&pldev->dev, sizeof(struct kp_spi)); +struct spi_master *master; struct kp_spi *kpspi; struct resource *r; int status = 0; -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/2] staging: kpc2000_spi: eliminated duplicate initialization of two local variables.
A couple of the local variables in kp_spi_probe had initializers which were being overwritten by immediate assignment of the same values. One of them was dynamically allocated and so would leak memory. Checkpatch whinges about the formatting, but fixing that would be rather a large job. Jeremy Sowden (2): staging: kpc2000_spi: eliminated duplicate initialization of drvdata local variable. staging: kpc2000_spi: eliminated duplicate initialization of master local variable. drivers/staging/kpc2000/kpc_spi/spi_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: kpc2000_spi: eliminated duplicate initialization of drvdata local variable.
drvdata was being initialized to a particular value and then having the same value assigned to it immediately afterwards. Removed the initializer. Since the value assigned, pldev->dev.platform_data, is a pointer-to- void, removed superfluous cast. Signed-off-by: Jeremy Sowden --- drivers/staging/kpc2000/kpc_spi/spi_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/kpc2000/kpc_spi/spi_driver.c b/drivers/staging/kpc2000/kpc_spi/spi_driver.c index 63b4616bf538..e77f04bf02d9 100644 --- a/drivers/staging/kpc2000/kpc_spi/spi_driver.c +++ b/drivers/staging/kpc2000/kpc_spi/spi_driver.c @@ -407,14 +407,14 @@ kp_spi_cleanup(struct spi_device *spidev) static int kp_spi_probe(struct platform_device *pldev) { -struct kpc_core_device_platdata *drvdata = (struct kpc_core_device_platdata *)pldev->dev.platform_data; +struct kpc_core_device_platdata *drvdata; struct spi_master *master = spi_alloc_master(&pldev->dev, sizeof(struct kp_spi)); struct kp_spi *kpspi; struct resource *r; int status = 0; int i; -drvdata = (struct kpc_core_device_platdata *)pldev->dev.platform_data; +drvdata = pldev->dev.platform_data; if (!drvdata){ dev_err(&pldev->dev, "kp_spi_probe: platform_data is NULL!\n"); return -ENODEV; -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vc04_services: bcm2835-camera: Modify return statement.
Am 27.04.19 um 18:07 schrieb Vatsala Narang: > Modify return statement and remove the respective assignment. > > Issue found by Coccinelle. > > Signed-off-by: Vatsala Narang Acked-by: Stefan Wahren ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] Staging: vc04_services: Cleanup in ctrl_set_bitrate()
Am 23.04.19 um 16:47 schrieb Madhumitha Prabakaran: > Remove unnecessary variable from the function and make a corresponding > change w.r.t the variable. In addition to that align the parameters in > the parentheses to maintain Linux kernel coding style > > Issue suggested by Coccinelle. > > Signed-off-by: Madhumitha Prabakaran Acked-by: Stefan Wahren ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: fieldbus: anybus-s: force endiannes annotation
While the endiannes is being handled correctly sparse was unhappy with the missing annotation as be16_to_cpu() expects a __be16. Signed-off-by: Nicholas Mc Guire --- Problem reported by sparse As far as I understand sparse here the __force is actually the only solution possible to inform sparse that the endiannes handling is ok Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m, HMS_ANYBUSS_BUS=m Patch is against 5.1-rc6 (localversion-next is next-20190426) drivers/staging/fieldbus/anybuss/host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c index 6227daf..278acac 100644 --- a/drivers/staging/fieldbus/anybuss/host.c +++ b/drivers/staging/fieldbus/anybuss/host.c @@ -1348,7 +1348,7 @@ anybuss_host_common_probe(struct device *dev, add_device_randomness(&val, 4); regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type, sizeof(fieldbus_type)); - fieldbus_type = be16_to_cpu(fieldbus_type); + fieldbus_type = be16_to_cpu((__force __be16)fieldbus_type); dev_info(dev, "Fieldbus type: %04X", fieldbus_type); regmap_bulk_read(cd->regmap, REG_MODULE_SW_V, val, 2); dev_info(dev, "Module SW version: %02X%02X", -- 2.1.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling
wait_for_completion_timeout() returns unsigned long (0 on timeout or remaining jiffies) not int - so rather than introducing an additional variable simply wrap the completion into an if(). Signed-off-by: Nicholas Mc Guire --- Problem located with experimental API conformance checking cocci script V2: The original patch's logic was wrong as it was skipping the fall-through if so using the fix proposed by Sven Van Asbroeck - This solution also eliminates the need to introduce an additional variable - Thanks ! Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m, HMS_ANYBUSS_BUS=m (with an unrelated sparse warnings (cast to restricted __be16)) Patch is against 5.1-rc6 (localversion-next is next-20190426) drivers/staging/fieldbus/anybuss/host.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c index e34d424..6227daf 100644 --- a/drivers/staging/fieldbus/anybuss/host.c +++ b/drivers/staging/fieldbus/anybuss/host.c @@ -1325,11 +1325,11 @@ anybuss_host_common_probe(struct device *dev, * interrupt came in: ready to go ! */ reset_deassert(cd); - ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT); - if (ret == 0) + if (!wait_for_completion_timeout(&cd->card_boot, TIMEOUT)) { ret = -ETIMEDOUT; - if (ret < 0) goto err_reset; + } + /* * according to the anybus docs, we're allowed to read these * without handshaking / reserving the area -- 2.1.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel