Re: [PATCH 55/57] staging: comedi: drivers: ni_mio_common: Move 'range_ni_E_ao_ext' to where it is used
On 14/04/2021 19:11, Lee Jones wrote:
... and mark it as __maybe_unused since not all users of the
header file reference it.
Fixes the following W=1 kernel build warning(s):
drivers/staging/comedi/drivers/ni_mio_common.c:163:35: warning:
‘range_ni_E_ao_ext’ defined but not used [-Wunused-const-variable=]
Cc: Ian Abbott
Cc: H Hartley Sweeten
Cc: Greg Kroah-Hartman
Cc: Thierry Reding
Cc: "Uwe Kleine-König"
Cc: Lee Jones
Cc: "David A. Schleef"
Cc: Mori Hess
Cc: Truxton Fulton
Cc: linux-stag...@lists.linux.dev
Cc: linux-...@vger.kernel.org
Signed-off-by: Lee Jones
---
drivers/staging/comedi/drivers/ni_mio_common.c | 9 -
drivers/staging/comedi/drivers/ni_stc.h| 9 -
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 4f80a4991f953..37615b4e2c10d 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -160,15 +160,6 @@ static const struct comedi_lrange range_ni_M_ai_628x = {
}
};
-static const struct comedi_lrange range_ni_E_ao_ext = {
- 4, {
- BIP_RANGE(10),
- UNI_RANGE(10),
- RANGE_ext(-1, 1),
- RANGE_ext(0, 1)
- }
-};
-
static const struct comedi_lrange *const ni_range_lkup[] = {
[ai_gain_16] = _ni_E_ai,
[ai_gain_8] = _ni_E_ai_limited,
diff --git a/drivers/staging/comedi/drivers/ni_stc.h
b/drivers/staging/comedi/drivers/ni_stc.h
index fbc0b753a0f59..0822e65f709dd 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -1137,6 +1137,13 @@ struct ni_private {
u8 rgout0_usage;
};
-static const struct comedi_lrange range_ni_E_ao_ext;
+static const struct comedi_lrange __maybe_unused range_ni_E_ao_ext = {
+ 4, {
+ BIP_RANGE(10),
+ UNI_RANGE(10),
+ RANGE_ext(-1, 1),
+ RANGE_ext(0, 1)
+ }
+};
#endif /* _COMEDI_NI_STC_H */
I think it is better where it is for now with its fellow struct
comedi_lrange variables, but feel free to mark it as __maybe_unused.
(Really, the #include "ni_mio_common.c" mess needs sorting out sometime.)
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH 48/57] staging: comedi: drivers: jr3_pci: Remove set but unused variable 'min_full_scale'
On 14/04/2021 19:11, Lee Jones wrote:
Fixes the following W=1 kernel build warning(s):
drivers/staging/comedi/drivers/jr3_pci.c: In function
‘jr3_pci_poll_subdevice’:
drivers/staging/comedi/drivers/jr3_pci.c:507:22: warning: variable
‘min_full_scale’ set but not used [-Wunused-but-set-variable]
Cc: Ian Abbott
Cc: H Hartley Sweeten
Cc: Greg Kroah-Hartman
Cc: "Alexander A. Klimov"
Cc: Anders Blomdell
Cc: linux-stag...@lists.linux.dev
Signed-off-by: Lee Jones
---
drivers/staging/comedi/drivers/jr3_pci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/jr3_pci.c
b/drivers/staging/comedi/drivers/jr3_pci.c
index 7a02c4fa3cda8..afa2f8d5c8c0c 100644
--- a/drivers/staging/comedi/drivers/jr3_pci.c
+++ b/drivers/staging/comedi/drivers/jr3_pci.c
@@ -504,10 +504,9 @@ jr3_pci_poll_subdevice(struct comedi_subdevice *s)
result = poll_delay_min_max(20, 100);
} else {
/* Set full scale */
- struct six_axis_t min_full_scale;
struct six_axis_t max_full_scale;
- min_full_scale = get_min_full_scales(sensor);
+ get_min_full_scales(sensor);
max_full_scale = get_max_full_scales(sensor);
set_full_scales(sensor, max_full_scale);
get_min_full_scales() could be removed altogether. It was only being
called originally so the driver could printk the values, but those
printks have since been removed.
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: mmotm 2021-04-11-20-47 uploaded (ni_routes_test.c)
: ni_routes_test.c:(.text+0xb3d): undefined reference to `ni_find_route_set'
ld: ni_routes_test.c:(.text+0xb74): undefined reference to `ni_find_route_set'
ld: ni_routes_test.c:(.text+0xbb2): undefined reference to `ni_find_route_set'
ld: ni_routes_test.c:(.text+0xbfa): undefined reference to `ni_find_route_set'
ld: drivers/staging/comedi/drivers/tests/ni_routes_test.o: in function
`test_ni_assign_device_routes':
ni_routes_test.c:(.text+0xc6c): undefined reference to `ni_assign_device_routes'
ld: ni_routes_test.c:(.text+0xeb6): undefined reference to
`ni_assign_device_routes'
ld: drivers/staging/comedi/drivers/tests/ni_routes_test.o: in function
`test_ni_count_valid_routes':
ni_routes_test.c:(.text+0xf9c): undefined reference to `ni_count_valid_routes'
ld: drivers/staging/comedi/drivers/tests/ni_routes_test.o: in function
`ni_get_reg_value_roffs.constprop.7':
ni_routes_test.c:(.text+0xfef): undefined reference to `ni_find_route_source'
ld: ni_routes_test.c:(.text+0x1015): undefined reference to
`ni_route_to_register'
ld: drivers/staging/comedi/drivers/tests/ni_routes_test.o: in function
`test_route_is_valid':
ni_routes_test.c:(.text+0x1074): undefined reference to `ni_route_to_register'
ld: ni_routes_test.c:(.text+0x10ab): undefined reference to
`ni_route_to_register'
ld: ni_routes_test.c:(.text+0x10e2): undefined reference to
`ni_route_to_register'
ld: ni_routes_test.c:(.text+0x1119): undefined reference to
`ni_route_to_register'
ld: drivers/staging/comedi/drivers/tests/ni_routes_test.o: in function
`test_route_register_is_valid':
ni_routes_test.c:(.text+0x115a): undefined reference to `ni_find_route_source'
ld: ni_routes_test.c:(.text+0x118e): undefined reference to
`ni_find_route_source'
ld: ni_routes_test.c:(.text+0x11c5): undefined reference to
`ni_find_route_source'
ld: ni_routes_test.c:(.text+0x11fc): undefined reference to
`ni_find_route_source'
ld: drivers/staging/comedi/drivers/tests/ni_routes_test.o: in function
`test_ni_sort_device_routes':
ni_routes_test.c:(.text+0x16ef): undefined reference to `ni_sort_device_routes'
Full randconfig file is attached.
Hi all,
That should be fixed by commit e7442ffe1cc5 ("staging: comedi: Kconfig:
Fix COMEDI_TESTS_NI_ROUTES selections") in linux-next master.
Sorry for the trouble!
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] Staging: Remove line to fix checkpatch error
On 11/04/2021 21:49, tawahpeggy wrote: remove one empty line.CHECK: Please don't use multiple blank lines Signed-off-by: tawahpeggy --- drivers/staging/comedi/comedi_pcmcia.mod.c | 1 - 1 file changed, 0 insertion(+), 1 deletion(-) create mode 100644 drivers/staging/comedi/comedi_pcmcia.mod.c diff --git a/drivers/staging/comedi/comedi_pcmcia.mod.c b/drivers/staging/comedi/comedi_pcmcia.mod.c index 0904b8765afs96..3984db1a39c8 --- /dev/null +++ b/drivers/staging/comedi/comedi_pcmcia.mod.c The .mod.c files are not really part of the Linux kernel source code. They are generated during the kernel build process. There is no point checking them with checkpatch.pl. If you are adding them to your git repository, then you are doing something wrong. -- -=( Ian Abbott || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: das800: fix request_irq() warn
On 16/03/2021 22:42, Tong Zhang wrote:
request_irq() wont accept a name which contains slash so we need to
repalce it with something else -- otherwise it will trigger a warning
and the entry in /proc/irq/ will not be created
since the .name might be used by userspace and we don't want to break
userspace, so we are changing the parameters passed to request_irq()
Suggested-by: Ian Abbott
Signed-off-by: Tong Zhang
---
drivers/staging/comedi/drivers/das800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/das800.c
b/drivers/staging/comedi/drivers/das800.c
index 2881808d6606..bc08324f422f 100644
--- a/drivers/staging/comedi/drivers/das800.c
+++ b/drivers/staging/comedi/drivers/das800.c
@@ -668,7 +668,7 @@ static int das800_attach(struct comedi_device *dev, struct
comedi_devconfig *it)
dev->board_name = board->name;
if (irq > 1 && irq <= 7) {
- ret = request_irq(irq, das800_interrupt, 0, dev->board_name,
+ ret = request_irq(irq, das800_interrupt, 0, "das800",
dev);
if (ret == 0)
dev->irq = irq;
Looks good (apart from the minor spelling niggle spotted by Dan
Carpenter), thanks!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: replace slash in name
On 15/03/2021 20:00, Tong Zhang wrote: Thanks Ian, I have submitted a v2 patch based on your suggestions. Thanks, - Tong Thanks. I think the only other Comedi driver with the same problem is "drivers/staging/comedi/drivers/das800.c". It passes dev->board_name as the name argument of request_irq(), but that is "cio-das802/16" for one of the boards supported by the driver. On Mon, Mar 15, 2021 at 6:48 AM Ian Abbott wrote: On 15/03/2021 10:44, Ian Abbott wrote: On 14/03/2021 03:57, Tong Zhang wrote: request_irq() wont accept a name which contains slash so we need to repalce it with something else -- otherwise it will trigger a warning and the entry in /proc/irq/ will not be created [1.565966] name 'pci-das6402/16' [1.566149] WARNING: CPU: 0 PID: 184 at fs/proc/generic.c:180 __xlate_proc_name+0x93/0xb0 [1.568923] RIP: 0010:__xlate_proc_name+0x93/0xb0 [1.574200] Call Trace: [1.574722] proc_mkdir+0x18/0x20 [1.576629] request_threaded_irq+0xfe/0x160 [1.576859] auto_attach+0x60a/0xc40 [cb_pcidas64] Signed-off-by: Tong Zhang [snip] Userspace applications can use these strings to determine the board type, so changing the strings would break those applications. I suggest passing the comedi driver name "cb_pcidas" to request_irq() for now. Oops, I meant "cb_pcidas64". But you could reach that via dev->driver->driver_name if you want (where dev is the struct comedi_device * parameter). -- -=( Ian Abbott || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH v2] staging: comedi: cb_pcidas64: fix request_irq() warn
On 15/03/2021 19:58, Tong Zhang wrote:
request_irq() wont accept a name which contains slash so we need to
repalce it with something else -- otherwise it will trigger a warning
and the entry in /proc/irq/ will not be created
since the .name might be used by userspace and we don't want to break
userspace, so we are changing the parameters passed to request_irq()
[1.565966] name 'pci-das6402/16'
[1.566149] WARNING: CPU: 0 PID: 184 at fs/proc/generic.c:180
__xlate_proc_name+0x93/0xb0
[1.568923] RIP: 0010:__xlate_proc_name+0x93/0xb0
[1.574200] Call Trace:
[1.574722] proc_mkdir+0x18/0x20
[1.576629] request_threaded_irq+0xfe/0x160
[1.576859] auto_attach+0x60a/0xc40 [cb_pcidas64]
Suggested-by: Ian Abbott
Signed-off-by: Tong Zhang
---
v2: revert changes to .name field so that we dont break userspace
drivers/staging/comedi/drivers/cb_pcidas64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index fa987bb0e7cd..6d3ba399a7f0 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -4035,7 +4035,7 @@ static int auto_attach(struct comedi_device *dev,
init_stc_registers(dev);
retval = request_irq(pcidev->irq, handle_interrupt, IRQF_SHARED,
-dev->board_name, dev);
+"cb_pcidas64", dev);
if (retval) {
dev_dbg(dev->class_dev, "unable to allocate irq %u\n",
pcidev->irq);
Looks good.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH v2] staging: comedi: cb_pcidas: fix request_irq() warn
On 15/03/2021 19:59, Tong Zhang wrote:
request_irq() wont accept a name which contains slash so we need to
repalce it with something else -- otherwise it will trigger a warning
and the entry in /proc/irq/ will not be created
since the .name might be used by userspace and we don't want to break
userspace, so we are changing the parameters passed to request_irq()
[1.630764] name 'pci-das1602/16'
[1.630950] WARNING: CPU: 0 PID: 181 at fs/proc/generic.c:180
__xlate_proc_name+0x93/0xb0
[1.634009] RIP: 0010:__xlate_proc_name+0x93/0xb0
[1.639441] Call Trace:
[1.639976] proc_mkdir+0x18/0x20
[1.641946] request_threaded_irq+0xfe/0x160
[1.642186] cb_pcidas_auto_attach+0xf4/0x610 [cb_pcidas]
Suggested-by: Ian Abbott
Signed-off-by: Tong Zhang
---
v2: revert changes to .name field so that we dont break userspace
drivers/staging/comedi/drivers/cb_pcidas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/cb_pcidas.c
b/drivers/staging/comedi/drivers/cb_pcidas.c
index d740c4782775..2f20bd56ec6c 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas.c
@@ -1281,7 +1281,7 @@ static int cb_pcidas_auto_attach(struct comedi_device
*dev,
devpriv->amcc + AMCC_OP_REG_INTCSR);
ret = request_irq(pcidev->irq, cb_pcidas_interrupt, IRQF_SHARED,
- dev->board_name, dev);
+ "cb_pcidas", dev);
if (ret) {
dev_dbg(dev->class_dev, "unable to allocate irq %d\n",
pcidev->irq);
Looks good.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: cb_pcidas: replace slash in name
On 14/03/2021 04:02, Tong Zhang wrote:
> request_irq() wont accept a name which contains slash so we need to
> repalce it with something else -- otherwise it will trigger a warning
> and the entry in /proc/irq/ will not be created
>
> [1.630764] name 'pci-das1602/16'
> [1.630950] WARNING: CPU: 0 PID: 181 at fs/proc/generic.c:180
> __xlate_proc_name+0x93/0xb0
> [1.634009] RIP: 0010:__xlate_proc_name+0x93/0xb0
> [1.639441] Call Trace:
> [1.639976] proc_mkdir+0x18/0x20
> [1.641946] request_threaded_irq+0xfe/0x160
> [1.642186] cb_pcidas_auto_attach+0xf4/0x610 [cb_pcidas]
>
> Signed-off-by: Tong Zhang
> ---
> drivers/staging/comedi/drivers/cb_pcidas.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/cb_pcidas.c
> b/drivers/staging/comedi/drivers/cb_pcidas.c
> index d740c4782775..df0960d41cff 100644
> --- a/drivers/staging/comedi/drivers/cb_pcidas.c
> +++ b/drivers/staging/comedi/drivers/cb_pcidas.c
> @@ -230,7 +230,7 @@ struct cb_pcidas_board {
>
> static const struct cb_pcidas_board cb_pcidas_boards[] = {
> [BOARD_PCIDAS1602_16] = {
> - .name = "pci-das1602/16",
> + .name = "pci-das1602-16",
> .ai_speed = 5000,
> .ao_scan_speed = 1,
> .fifo_size = 512,
> @@ -248,7 +248,7 @@ static const struct cb_pcidas_board cb_pcidas_boards[] = {
> .has_ao = 1,
> },
> [BOARD_PCIDAS1602_12] = {
> - .name = "pci-das1602/12",
> + .name = "pci-das1602-12",
> .ai_speed = 3200,
> .ao_scan_speed = 4000,
> .fifo_size = 1024,
> @@ -257,12 +257,12 @@ static const struct cb_pcidas_board cb_pcidas_boards[]
> = {
> .is_1602= 1,
> },
> [BOARD_PCIDAS1200_JR] = {
> - .name = "pci-das1200/jr",
> + .name = "pci-das1200-jr",
> .ai_speed = 3200,
> .fifo_size = 1024,
> },
> [BOARD_PCIDAS1602_16_JR] = {
> - .name = "pci-das1602/16/jr",
> + .name = "pci-das1602-16-jr",
> .ai_speed = 5000,
> .fifo_size = 512,
> .is_16bit = 1,
>
As for cb_pcidas64, the board name may be used by user-space
applications, so this may break those applications.
I suggest passing the comedi driver name "cb_pcidas" to request_irq()
for now. (It can also be reached via dev->driver->driver_name .)
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: replace slash in name
On 15/03/2021 10:44, Ian Abbott wrote: > On 14/03/2021 03:57, Tong Zhang wrote: >> request_irq() wont accept a name which contains slash so we need to >> repalce it with something else -- otherwise it will trigger a warning >> and the entry in /proc/irq/ will not be created >> >> [1.565966] name 'pci-das6402/16' >> [1.566149] WARNING: CPU: 0 PID: 184 at fs/proc/generic.c:180 >> __xlate_proc_name+0x93/0xb0 >> [1.568923] RIP: 0010:__xlate_proc_name+0x93/0xb0 >> [1.574200] Call Trace: >> [1.574722] proc_mkdir+0x18/0x20 >> [1.576629] request_threaded_irq+0xfe/0x160 >> [1.576859] auto_attach+0x60a/0xc40 [cb_pcidas64] >> >> Signed-off-by: Tong Zhang [snip] > Userspace applications can use these strings to determine the board > type, so changing the strings would break those applications. > > I suggest passing the comedi driver name "cb_pcidas" to request_irq() > for now. Oops, I meant "cb_pcidas64". But you could reach that via dev->driver->driver_name if you want (where dev is the struct comedi_device * parameter). -- -=( Ian Abbott || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: replace slash in name
On 14/03/2021 03:57, Tong Zhang wrote:
> request_irq() wont accept a name which contains slash so we need to
> repalce it with something else -- otherwise it will trigger a warning
> and the entry in /proc/irq/ will not be created
>
> [1.565966] name 'pci-das6402/16'
> [1.566149] WARNING: CPU: 0 PID: 184 at fs/proc/generic.c:180
> __xlate_proc_name+0x93/0xb0
> [1.568923] RIP: 0010:__xlate_proc_name+0x93/0xb0
> [1.574200] Call Trace:
> [1.574722] proc_mkdir+0x18/0x20
> [1.576629] request_threaded_irq+0xfe/0x160
> [1.576859] auto_attach+0x60a/0xc40 [cb_pcidas64]
>
> Signed-off-by: Tong Zhang
> ---
> drivers/staging/comedi/drivers/cb_pcidas64.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c
> b/drivers/staging/comedi/drivers/cb_pcidas64.c
> index fa987bb0e7cd..662d6ffb8f60 100644
> --- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> +++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> @@ -677,7 +677,7 @@ static const int bytes_in_sample = 2;
>
> static const struct pcidas64_board pcidas64_boards[] = {
> [BOARD_PCIDAS6402_16] = {
> - .name = "pci-das6402/16",
> + .name = "pci-das6402-16",
> .ai_se_chans= 64,
> .ai_bits= 16,
> .ai_speed = 5000,
> @@ -693,7 +693,7 @@ static const struct pcidas64_board pcidas64_boards[] = {
> .has_8255 = 1,
> },
> [BOARD_PCIDAS6402_12] = {
> - .name = "pci-das6402/12", /* XXX check */
> + .name = "pci-das6402-12", /* XXX check */
> .ai_se_chans= 64,
> .ai_bits= 12,
> .ai_speed = 5000,
> @@ -709,7 +709,7 @@ static const struct pcidas64_board pcidas64_boards[] = {
> .has_8255 = 1,
> },
> [BOARD_PCIDAS64_M1_16] = {
> - .name = "pci-das64/m1/16",
> + .name = "pci-das64-m1-16",
> .ai_se_chans= 64,
> .ai_bits= 16,
> .ai_speed = 1000,
> @@ -725,7 +725,7 @@ static const struct pcidas64_board pcidas64_boards[] = {
> .has_8255 = 1,
> },
> [BOARD_PCIDAS64_M2_16] = {
> - .name = "pci-das64/m2/16",
> + .name = "pci-das64-m2-16",
> .ai_se_chans= 64,
> .ai_bits= 16,
> .ai_speed = 500,
> @@ -741,7 +741,7 @@ static const struct pcidas64_board pcidas64_boards[] = {
> .has_8255 = 1,
> },
> [BOARD_PCIDAS64_M3_16] = {
> - .name = "pci-das64/m3/16",
> + .name = "pci-das64-m3-16",
> .ai_se_chans= 64,
> .ai_bits= 16,
> .ai_speed = 333,
> @@ -984,7 +984,7 @@ static const struct pcidas64_board pcidas64_boards[] = {
> .has_8255 = 0,
> },
> [BOARD_PCIDAS4020_12] = {
> - .name = "pci-das4020/12",
> + .name = "pci-das4020-12",
> .ai_se_chans= 4,
> .ai_bits= 12,
> .ai_speed = 50,
>
Userspace applications can use these strings to determine the board
type, so changing the strings would break those applications.
I suggest passing the comedi driver name "cb_pcidas" to request_irq()
for now.
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH -next] staging: comedi dt2814: Removed unused variables
On 21/02/2021 20:28, Fatih Yildirim wrote:
> Removed unused variables.
>
> Signed-off-by: Fatih Yildirim
> ---
> drivers/staging/comedi/drivers/dt2814.c | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/dt2814.c
> b/drivers/staging/comedi/drivers/dt2814.c
> index bcf4d5444faf..bd329d7b4893 100644
> --- a/drivers/staging/comedi/drivers/dt2814.c
> +++ b/drivers/staging/comedi/drivers/dt2814.c
> @@ -186,21 +186,17 @@ static int dt2814_ai_cmd(struct comedi_device *dev,
> struct comedi_subdevice *s)
>
> static irqreturn_t dt2814_interrupt(int irq, void *d)
> {
> - int lo, hi;
> struct comedi_device *dev = d;
> struct dt2814_private *devpriv = dev->private;
> struct comedi_subdevice *s = dev->read_subdev;
> - int data;
>
> if (!dev->attached) {
> dev_err(dev->class_dev, "spurious interrupt\n");
> return IRQ_HANDLED;
> }
>
> - hi = inb(dev->iobase + DT2814_DATA);
> - lo = inb(dev->iobase + DT2814_DATA);
> -
> - data = (hi << 4) | (lo >> 4);
> + inb(dev->iobase + DT2814_DATA);
> + inb(dev->iobase + DT2814_DATA);
>
> if (!(--devpriv->ntrig)) {
> int i;
> @@ -229,7 +225,6 @@ static int dt2814_attach(struct comedi_device *dev,
> struct comedi_devconfig *it)
> struct dt2814_private *devpriv;
> struct comedi_subdevice *s;
> int ret;
> - int i;
>
> ret = comedi_request_region(dev, it->options[0], 0x2);
> if (ret)
> @@ -241,8 +236,8 @@ static int dt2814_attach(struct comedi_device *dev,
> struct comedi_devconfig *it)
> dev_err(dev->class_dev, "reset error (fatal)\n");
> return -EIO;
> }
> - i = inb(dev->iobase + DT2814_DATA);
> - i = inb(dev->iobase + DT2814_DATA);
> + inb(dev->iobase + DT2814_DATA);
> + inb(dev->iobase + DT2814_DATA);
>
> if (it->options[1]) {
> ret = request_irq(it->options[1], dt2814_interrupt, 0,
>
I've no objection to this patch. The interrupt handling to support
Comedi asynchronous commands in this driver has always been broken. I'm
tempted to remove the code for handling asynchronous commands in this
driver altogether for that reason. (The naive fix of writing the data
to the Comedi buffer is insufficient without an additional check that
the command is running. The end-of-conversion interrupt occurs
regardless of any command being active.)
Acked-by: Ian Abbott
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: cast to (unsigned int *)
On 19/02/2021 09:03, David Laight wrote: >> It's kind of moot anyway because the patch is outdated. But the reason >> for the ___force is that the same `struct comedi_cmd` is used in both >> user and kernel contexts. In user contexts, the `chanlist` member >> points to user memory and in kernel contexts it points to kernel memory >> (copied from userspace). > > Can't you use a union of the user and kernel pointers? > (Possibly even anonymous?) > Although, ideally, keeping them in separate fields is better. > 8 bytes for a pointer isn't going make a fat lot of difference. This is for a UAPI header (eventually), so cannot add a new field. For an anonymous union, one tagged with __user and one not, the __user tag would be removed during conversion from UAPI headers to /usr/include/linux headers, leaving a union of two identically typed members, which would look a bit odd. The union also kind of hides the problem. -- -=( Ian Abbott || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] Staging: comedi: Replaced strlcpy to strscpy
On 18/02/2021 14:31, chakravarthikulkarni wrote: > Warning found by checkpath.pl script. > > Signed-off-by: chakravarthikulkarni > --- > drivers/staging/comedi/comedi_fops.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/comedi/comedi_fops.c > b/drivers/staging/comedi/comedi_fops.c > index 80d74cce2a01..df77b6bf5c64 100644 > --- a/drivers/staging/comedi/comedi_fops.c > +++ b/drivers/staging/comedi/comedi_fops.c > @@ -939,8 +939,8 @@ static int do_devinfo_ioctl(struct comedi_device *dev, > /* fill devinfo structure */ > devinfo.version_code = COMEDI_VERSION_CODE; > devinfo.n_subdevs = dev->n_subdevices; > - strlcpy(devinfo.driver_name, dev->driver->driver_name, COMEDI_NAMELEN); > - strlcpy(devinfo.board_name, dev->board_name, COMEDI_NAMELEN); > + strscpy(devinfo.driver_name, dev->driver->driver_name, COMEDI_NAMELEN); > + strscpy(devinfo.board_name, dev->board_name, COMEDI_NAMELEN); > > s = comedi_file_read_subdevice(file); > if (s) > Thanks, but you are too late. It has already been fixed in linux-next. -- -=( Ian Abbott || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] staging: comedi: cast to (unsigned int *)
On 17/02/2021 18:26, Greg KH wrote:
On Wed, Feb 17, 2021 at 11:40:00PM +0530, Atul Gopinathan wrote:
On Wed, Feb 17, 2021 at 06:35:15PM +0100, Greg KH wrote:
On Wed, Feb 17, 2021 at 10:29:08PM +0530, Atul Gopinathan wrote:
Resolve the following warning generated by sparse:
drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in
assignment (different address spaces)
drivers/staging//comedi/comedi_fops.c:2956:23:expected unsigned int
*chanlist
drivers/staging//comedi/comedi_fops.c:2956:23:got void [noderef] *
compat_ptr() has a return type of "void __user *"
as defined in "include/linux/compat.h"
cmd->chanlist is of type "unsigned int *" as defined
in drivers/staging/comedi/comedi.h" in struct
comedi_cmd.
Signed-off-by: Atul Gopinathan
---
drivers/staging/comedi/comedi_fops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index e85a99b68f31..fc4ec38012b4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd,
cmd->scan_end_arg = v32.scan_end_arg;
cmd->stop_src = v32.stop_src;
cmd->stop_arg = v32.stop_arg;
- cmd->chanlist = compat_ptr(v32.chanlist);
+ cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist);
__force? That feels wrong, something is odd if that is ever needed.
Are you _sure_ this is correct?
The same file has instances of "(usigned int __force *)" cast being
used on the same "cmd->chanlist". For reference:
At line 1797 of comedi_fops.c:
1796 /* restore chanlist pointer before copying back */
1797 cmd->chanlist = (unsigned int __force *)user_chanlist;
1798 cmd->data = NULL;
At line 1880:
1879 /* restore chanlist pointer before copying back */
1880 cmd->chanlist = (unsigned int __force *)user_chanlist;
1881 *copy = true;
Here "user_chanlist" is of type "unsigned int __user *".
Or perhaps, I shouldn't be relying on them?
I don't know, it still feels wrong.
Ian, any thoughts?
It's kind of moot anyway because the patch is outdated. But the reason
for the ___force is that the same `struct comedi_cmd` is used in both
user and kernel contexts. In user contexts, the `chanlist` member
points to user memory and in kernel contexts it points to kernel memory
(copied from userspace).
The sparse tagging of this member has flip-flopped a bit over the years:
* commit 92d0127c9d24 ("Staging: comedi: __user markup on
comedi_fops.c") (May 2010) tagged it as `__user`.
* commit 9be56c643263 ("staging: comedi: comedi.h: remove __user tag
from chanlist") (Sep 2012) removed the `__user` tag.
It is mostly used in a kernel context, for example all the low-level
drivers with `do_cmd` and `do_cmdtest` handlers use it in kernel context.
The alternative would be to have a separate kernel version of this
struct, but it would be mostly identical to the user version apart from
the sparse tagging of this member and perhaps the removal of the unused
`data` and `data_len` members (which need to be kept in the user version
of the struct for compatibility reasons).
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH v2 1/2] staging: comedi: cast function output to assigned variable type
On 18/02/2021 08:44, Atul Gopinathan wrote:
Fix the following warning generated by sparse:
drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in
assignment (different address spaces)
drivers/staging//comedi/comedi_fops.c:2956:23:expected unsigned int
*chanlist
drivers/staging//comedi/comedi_fops.c:2956:23:got void [noderef] *
compat_ptr() has a return type of "void __user *"
as defined in "include/linux/compat.h"
cmd->chanlist is of type "unsigned int *" as defined
in drivers/staging/comedi/comedi.h" in struct
comedi_cmd.
Signed-off-by: Atul Gopinathan
---
drivers/staging/comedi/comedi_fops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index e85a99b68f31..fc4ec38012b4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd,
cmd->scan_end_arg = v32.scan_end_arg;
cmd->stop_src = v32.stop_src;
cmd->stop_arg = v32.stop_arg;
- cmd->chanlist = compat_ptr(v32.chanlist);
+ cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist);
cmd->chanlist_len = v32.chanlist_len;
cmd->data = compat_ptr(v32.data);
cmd->data_len = v32.data_len;
This patch and the other one in your series clash with commit
9d5d041eebe3 ("staging: comedi: comedi_fops.c: added casts to get rid of
sparse warnings") by B K Karthik.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9d5d041eebe3dcf7591ff7004896c329eb841ca6
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
Re: [PATCH] drivers: staging: comedi: Fixed side effects from macro definition.
On 17/02/2021 14:20, chakravarthikulkarni wrote:
Warning found by checkpatch.pl script.
Signed-off-by: chakravarthikulkarni
---
drivers/staging/comedi/comedi.h | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/comedi.h b/drivers/staging/comedi/comedi.h
index b5d00a006dbb..b2af6a88d389 100644
--- a/drivers/staging/comedi/comedi.h
+++ b/drivers/staging/comedi/comedi.h
@@ -1103,9 +1103,12 @@ enum ni_common_signal_names {
/* *** END GLOBALLY-NAMED NI TERMINALS/SIGNALS *** */
-#define NI_USUAL_PFI_SELECT(x) (((x) < 10) ? (0x1 + (x)) : (0xb + (x)))
-#define NI_USUAL_RTSI_SELECT(x)(((x) < 7) ? (0xb + (x)) : 0x1b)
-
+#define NI_USUAL_PFI_SELECT(x) \
+ ({ typeof(x) _x = x; \
+(((_x) < 10) ? (0x1 + (_x)) : (0xb + (_x))); })
+#define NI_USUAL_RTSI_SELECT(x)\
+ ({ typeof(x) _x = x; \
+(((_x) < 7) ? (0xb + (_x)) : 0x1b); })
/*
* mode bits for NI general-purpose counters, set with
* INSN_CONFIG_SET_COUNTER_MODE
I'd rather not do that because this is intended to be a userspace
header. This change adds GCC extensions and prohibits the use of the
macros in constant expressions.
--
-=( Ian Abbott || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
{PATCH 0/2] HID: wiimote: Minor change to spinlock usage
[I have posted these patches previously, but that was over a year ago. --IA] A couple of minor changes to the wiimote core module. They do not change functionality of the driver: 1) HID: wiimote: make handlers[] const 2) HID: wiimote: narrow spinlock range in wiimote_hid_event() drivers/hid/hid-wiimote-core.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-)
[PATCH 2/2] HID: wiimote: narrow spinlock range in wiimote_hid_event()
In `wiimote_hid_event()`, the `wdata->state.lock` spinlock does not need
to be held while searching `handlers[]` for a suitable handler function.
Change it so the spinlock is only held during the call to the handler
function itself.
Signed-off-by: Ian Abbott
---
drivers/hid/hid-wiimote-core.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index e03a0ba5611a..41012681cafd 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1625,12 +1625,12 @@ static int wiimote_hid_event(struct hid_device *hdev,
struct hid_report *report,
if (size < 1)
return -EINVAL;
- spin_lock_irqsave(>state.lock, flags);
-
for (i = 0; handlers[i].id; ++i) {
h = [i];
if (h->id == raw_data[0] && h->size < size) {
+ spin_lock_irqsave(>state.lock, flags);
h->func(wdata, _data[1]);
+ spin_unlock_irqrestore(>state.lock, flags);
break;
}
}
@@ -1639,8 +1639,6 @@ static int wiimote_hid_event(struct hid_device *hdev,
struct hid_report *report,
hid_warn(hdev, "Unhandled report %hhu size %d\n", raw_data[0],
size);
- spin_unlock_irqrestore(>state.lock, flags);
-
return 0;
}
--
2.28.0
[PATCH 1/2] HID: wiimote: make handlers[] const
The `handlers[]` array contents are never modified, so use the `const`
qualifier.
Signed-off-by: Ian Abbott
---
drivers/hid/hid-wiimote-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index e484c3618dec..e03a0ba5611a 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1586,7 +1586,7 @@ struct wiiproto_handler {
void (*func)(struct wiimote_data *wdata, const __u8 *payload);
};
-static struct wiiproto_handler handlers[] = {
+static const struct wiiproto_handler handlers[] = {
{ .id = WIIPROTO_REQ_STATUS, .size = 6, .func = handler_status },
{ .id = WIIPROTO_REQ_STATUS, .size = 2, .func = handler_status_K },
{ .id = WIIPROTO_REQ_DATA, .size = 21, .func = handler_data },
@@ -1618,7 +1618,7 @@ static int wiimote_hid_event(struct hid_device *hdev,
struct hid_report *report,
u8 *raw_data, int size)
{
struct wiimote_data *wdata = hid_get_drvdata(hdev);
- struct wiiproto_handler *h;
+ const struct wiiproto_handler *h;
int i;
unsigned long flags;
--
2.28.0
Re: [PATCH] drivers: staging: comedi: fixed duplicate words from checkpatch
-286,7 +286,7 @@ int ni_tio_cmdtest(struct comedi_device *dev,
* This should be done, but we don't yet know the actual
* register values. These should be tested and then documented
* in the ni_route_values/ni_*.csv files, with indication of
-* who/when/which/how these these were tested.
+* who/when/which/how these were tested.
* When at least a e/m/660x series have been tested, this code
* should be uncommented:
*
diff --git a/drivers/staging/comedi/drivers/pcmuio.c
b/drivers/staging/comedi/drivers/pcmuio.c
index 7e1fc6ffb48c..b299d648a0eb 100644
--- a/drivers/staging/comedi/drivers/pcmuio.c
+++ b/drivers/staging/comedi/drivers/pcmuio.c
@@ -48,7 +48,7 @@
*
* In the 48-channel version:
*
- * On subdev 0, the first 24 channels channels are edge-detect channels.
+ * On subdev 0, the first 24 channels are edge-detect channels.
*
* In the 96-channel board you have the following channels that can do edge
* detection:
diff --git a/drivers/staging/comedi/drivers/quatech_daqp_cs.c
b/drivers/staging/comedi/drivers/quatech_daqp_cs.c
index 1b1efa4d31f6..fe4408ebf6b3 100644
--- a/drivers/staging/comedi/drivers/quatech_daqp_cs.c
+++ b/drivers/staging/comedi/drivers/quatech_daqp_cs.c
@@ -164,7 +164,7 @@ static int daqp_clear_events(struct comedi_device *dev, int
loops)
/*
* Reset any pending interrupts (my card has a tendency to require
-* require multiple reads on the status register to achieve this).
+* multiple reads on the status register to achieve this).
*/
while (--loops) {
status = inb(dev->iobase + DAQP_STATUS_REG);
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[PATCH 4/4] staging: comedi: addi_apci_1500: check INSN_CONFIG_DIGITAL_TRIG shift
The `INSN_CONFIG` comedi instruction with sub-instruction code
`INSN_CONFIG_DIGITAL_TRIG` includes a base channel in `data[3]`. This is
used as a right shift amount for other bitmask values without being
checked. Shift amounts greater than or equal to 32 will result in
undefined behavior. Add code to deal with this, adjusting the checks
for invalid channels so that enabled channel bits that would have been
lost by shifting are also checked for validity. Only channels 0 to 15
are valid.
Fixes: a8c66b684efaf ("staging: comedi: addi_apci_1500: rewrite the subdevice
support functions")
Cc: #4.0+: ef75e14a6c93: staging: comedi: verify array
index is correct before using it
Cc: #4.0+
Signed-off-by: Ian Abbott
---
.../staging/comedi/drivers/addi_apci_1500.c | 24 +++
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi_apci_1500.c
b/drivers/staging/comedi/drivers/addi_apci_1500.c
index 689acd69a1b9..816dd25b9d0e 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1500.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1500.c
@@ -452,13 +452,14 @@ static int apci1500_di_cfg_trig(struct comedi_device *dev,
struct apci1500_private *devpriv = dev->private;
unsigned int trig = data[1];
unsigned int shift = data[3];
- unsigned int hi_mask = data[4] << shift;
- unsigned int lo_mask = data[5] << shift;
- unsigned int chan_mask = hi_mask | lo_mask;
- unsigned int old_mask = (1 << shift) - 1;
+ unsigned int hi_mask;
+ unsigned int lo_mask;
+ unsigned int chan_mask;
+ unsigned int old_mask;
unsigned int pm;
unsigned int pt;
unsigned int pp;
+ unsigned int invalid_chan;
if (trig > 1) {
dev_dbg(dev->class_dev,
@@ -466,7 +467,20 @@ static int apci1500_di_cfg_trig(struct comedi_device *dev,
return -EINVAL;
}
- if (chan_mask > 0x) {
+ if (shift <= 16) {
+ hi_mask = data[4] << shift;
+ lo_mask = data[5] << shift;
+ old_mask = (1U << shift) - 1;
+ invalid_chan = (data[4] | data[5]) >> (16 - shift);
+ } else {
+ hi_mask = 0;
+ lo_mask = 0;
+ old_mask = 0x;
+ invalid_chan = data[4] | data[5];
+ }
+ chan_mask = hi_mask | lo_mask;
+
+ if (invalid_chan) {
dev_dbg(dev->class_dev, "invalid digital trigger channel\n");
return -EINVAL;
}
--
2.27.0
[PATCH 3/4] staging: comedi: addi_apci_1564: check INSN_CONFIG_DIGITAL_TRIG shift
The `INSN_CONFIG` comedi instruction with sub-instruction code
`INSN_CONFIG_DIGITAL_TRIG` includes a base channel in `data[3]`. This is
used as a right shift amount for other bitmask values without being
checked. Shift amounts greater than or equal to 32 will result in
undefined behavior. Add code to deal with this.
Fixes: 1e15687ea472 ("staging: comedi: addi_apci_1564: add Change-of-State
interrupt subdevice and required functions"
Cc: #3.17+
Signed-off-by: Ian Abbott
---
.../staging/comedi/drivers/addi_apci_1564.c | 20 +--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c
b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 10501fe6bb25..1268ba34be5f 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -331,14 +331,22 @@ static int apci1564_cos_insn_config(struct comedi_device
*dev,
unsigned int *data)
{
struct apci1564_private *devpriv = dev->private;
- unsigned int shift, oldmask;
+ unsigned int shift, oldmask, himask, lomask;
switch (data[0]) {
case INSN_CONFIG_DIGITAL_TRIG:
if (data[1] != 0)
return -EINVAL;
shift = data[3];
- oldmask = (1U << shift) - 1;
+ if (shift < 32) {
+ oldmask = (1U << shift) - 1;
+ himask = data[4] << shift;
+ lomask = data[5] << shift;
+ } else {
+ oldmask = 0xu;
+ himask = 0;
+ lomask = 0;
+ }
switch (data[2]) {
case COMEDI_DIGITAL_TRIG_DISABLE:
devpriv->ctrl = 0;
@@ -362,8 +370,8 @@ static int apci1564_cos_insn_config(struct comedi_device
*dev,
devpriv->mode2 &= oldmask;
}
/* configure specified channels */
- devpriv->mode1 |= data[4] << shift;
- devpriv->mode2 |= data[5] << shift;
+ devpriv->mode1 |= himask;
+ devpriv->mode2 |= lomask;
break;
case COMEDI_DIGITAL_TRIG_ENABLE_LEVELS:
if (devpriv->ctrl != (APCI1564_DI_IRQ_ENA |
@@ -380,8 +388,8 @@ static int apci1564_cos_insn_config(struct comedi_device
*dev,
devpriv->mode2 &= oldmask;
}
/* configure specified channels */
- devpriv->mode1 |= data[4] << shift;
- devpriv->mode2 |= data[5] << shift;
+ devpriv->mode1 |= himask;
+ devpriv->mode2 |= lomask;
break;
default:
return -EINVAL;
--
2.27.0
[PATCH 1/4] staging: comedi: ni_6527: fix INSN_CONFIG_DIGITAL_TRIG support
`ni6527_intr_insn_config()` processes `INSN_CONFIG` comedi instructions
for the "interrupt" subdevice. When `data[0]` is
`INSN_CONFIG_DIGITAL_TRIG` it is configuring the digital trigger. When
`data[2]` is `COMEDI_DIGITAL_TRIG_ENABLE_EDGES` it is configuring rising
and falling edge detection for the digital trigger, using a base channel
number (or shift amount) in `data[3]`, a rising edge bitmask in
`data[4]` and falling edge bitmask in `data[5]`.
If the base channel number (shift amount) is greater than or equal to
the number of channels (24) of the digital input subdevice, there are no
changes to the rising and falling edges, so the mask of channels to be
changed can be set to 0, otherwise the mask of channels to be changed,
and the rising and falling edge bitmasks are shifted by the base channel
number before calling `ni6527_set_edge_detection()` to change the
appropriate registers. Unfortunately, the code is comparing the base
channel (shift amount) to the interrupt subdevice's number of channels
(1) instead of the digital input subdevice's number of channels (24).
Fix it by comparing to 32 because all shift amounts for an `unsigned
int` must be less than that and everything from bit 24 upwards is
ignored by `ni6527_set_edge_detection()` anyway.
Fixes: 110f9e687c1a8 ("staging: comedi: ni_6527: support
INSN_CONFIG_DIGITAL_TRIG")
Cc: # 3.17+
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/ni_6527.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/ni_6527.c
b/drivers/staging/comedi/drivers/ni_6527.c
index 4d1eccb5041d..4518c2680b7c 100644
--- a/drivers/staging/comedi/drivers/ni_6527.c
+++ b/drivers/staging/comedi/drivers/ni_6527.c
@@ -332,7 +332,7 @@ static int ni6527_intr_insn_config(struct comedi_device
*dev,
case COMEDI_DIGITAL_TRIG_ENABLE_EDGES:
/* check shift amount */
shift = data[3];
- if (shift >= s->n_chan) {
+ if (shift >= 32) {
mask = 0;
rising = 0;
falling = 0;
--
2.27.0
[PATCH 2/4] staging: comedi: addi_apci_1032: check INSN_CONFIG_DIGITAL_TRIG shift
The `INSN_CONFIG` comedi instruction with sub-instruction code
`INSN_CONFIG_DIGITAL_TRIG` includes a base channel in `data[3]`. This is
used as a right shift amount for other bitmask values without being
checked. Shift amounts greater than or equal to 32 will result in
undefined behavior. Add code to deal with this.
Fixes: 33cdce6293dcc ("staging: comedi: addi_apci_1032: conform to new
INSN_CONFIG_DIGITAL_TRIG"
Cc: #3.8+
Signed-off-by: Ian Abbott
---
.../staging/comedi/drivers/addi_apci_1032.c | 20 +--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi_apci_1032.c
b/drivers/staging/comedi/drivers/addi_apci_1032.c
index 560649be9d13..e035c9f757a1 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1032.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1032.c
@@ -106,14 +106,22 @@ static int apci1032_cos_insn_config(struct comedi_device
*dev,
unsigned int *data)
{
struct apci1032_private *devpriv = dev->private;
- unsigned int shift, oldmask;
+ unsigned int shift, oldmask, himask, lomask;
switch (data[0]) {
case INSN_CONFIG_DIGITAL_TRIG:
if (data[1] != 0)
return -EINVAL;
shift = data[3];
- oldmask = (1U << shift) - 1;
+ if (shift < 32) {
+ oldmask = (1U << shift) - 1;
+ himask = data[4] << shift;
+ lomask = data[5] << shift;
+ } else {
+ oldmask = 0xu;
+ himask = 0;
+ lomask = 0;
+ }
switch (data[2]) {
case COMEDI_DIGITAL_TRIG_DISABLE:
devpriv->ctrl = 0;
@@ -136,8 +144,8 @@ static int apci1032_cos_insn_config(struct comedi_device
*dev,
devpriv->mode2 &= oldmask;
}
/* configure specified channels */
- devpriv->mode1 |= data[4] << shift;
- devpriv->mode2 |= data[5] << shift;
+ devpriv->mode1 |= himask;
+ devpriv->mode2 |= lomask;
break;
case COMEDI_DIGITAL_TRIG_ENABLE_LEVELS:
if (devpriv->ctrl != (APCI1032_CTRL_INT_ENA |
@@ -154,8 +162,8 @@ static int apci1032_cos_insn_config(struct comedi_device
*dev,
devpriv->mode2 &= oldmask;
}
/* configure specified channels */
- devpriv->mode1 |= data[4] << shift;
- devpriv->mode2 |= data[5] << shift;
+ devpriv->mode1 |= himask;
+ devpriv->mode2 |= lomask;
break;
default:
return -EINVAL;
--
2.27.0
[PATCH 0/4] staging: comedi: INSN_CONFIG_DIGITAL_TRIG fixes
These patches correct problems with INSN_CONFIG_DIGITAL_TRIG comedi
configuration instructions in various comedi drivers, in particular the
use of unconstrained bit shift amounts from userspace leading to
undefined behaviour (although hopefully not the kernel crashy sort).
The patches have been marked for inclusion in the stable tree. Note
that patch 4 changes a similar area of code to Dan Carpenter's commit
ef75e14a6c93 ("staging: comedi: verify array index is correct before
using it"), so I have indicated it as a prerequisite.
*Note to Greg KH*: I have based these patches on your "staging-linus"
branch due to the prerequisite ef75e14a6c93 mentioned above being
present in neither "staging-next" nor "staging-testing" at the time of
posting.
1) staging: comedi: ni_6527: fix INSN_CONFIG_DIGITAL_TRIG support
2) staging: comedi: addi_apci_1032: check INSN_CONFIG_DIGITAL_TRIG shift
3) staging: comedi: addi_apci_1564: check INSN_CONFIG_DIGITAL_TRIG shift
4) staging: comedi: addi_apci_1500: check INSN_CONFIG_DIGITAL_TRIG shift
drivers/staging/comedi/drivers/addi_apci_1032.c | 20 ++--
drivers/staging/comedi/drivers/addi_apci_1500.c | 24 +++-
drivers/staging/comedi/drivers/addi_apci_1564.c | 20 ++--
drivers/staging/comedi/drivers/ni_6527.c| 2 +-
4 files changed, 48 insertions(+), 18 deletions(-)
Re: [PATCH v4] staging: comedi: comedi_fops.c: added casts to get rid of sparse warnings
On 16/07/2020 16:25, B K Karthik wrote: fixed sparse warnings by adding a cast in assignment from void [noderef] __user * to unsigned int __force * and a reverse cast in argument from unsigned int * to unsigned int __user * . v1 -> v2: - Add a reverse cast in argument v2 -> v3: - Change commit description as suggested by Ian Abott v3 -> v4: - Add versioning information in commit description For future reference, the patch change history should go below the "---" divider line below where git will ignore it so that it does not appear in the final commit description. Otherwise, the committer would need to edit it out manually. Signed-off-by: B K Karthik --- drivers/staging/comedi/comedi_fops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 3f70e5dfac39..9cdc1e8a022d 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2956,7 +2956,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, cmd->scan_end_arg = v32.scan_end_arg; cmd->stop_src = v32.stop_src; cmd->stop_arg = v32.stop_arg; - cmd->chanlist = compat_ptr(v32.chanlist); + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); cmd->chanlist_len = v32.chanlist_len; cmd->data = compat_ptr(v32.data); cmd->data_len = v32.data_len; @@ -2983,7 +2983,7 @@ static int put_compat_cmd(struct comedi32_cmd_struct __user *cmd32, v32.stop_src = cmd->stop_src; v32.stop_arg = cmd->stop_arg; /* Assume chanlist pointer is unchanged. */ - v32.chanlist = ptr_to_compat(cmd->chanlist); + v32.chanlist = ptr_to_compat((unsigned int __user *)cmd->chanlist); v32.chanlist_len = cmd->chanlist_len; v32.data = ptr_to_compat(cmd->data); v32.data_len = cmd->data_len; Still looks good. Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH v2] staging: comedi: comedi_fops.c: added casts to get rid of sparse warnings
On 15/07/2020 12:48, B K Karthik wrote: fixed sparse warnings by adding a cast in assignment from void [noderef] __user * to unsigned int __force * and a reverse cast in argument from void [noderef] __user * to unsigned int __user * . Minor quibble: the reverse cast is actually from unsigned int * to unsigned int __user * . Signed-off-by: B K Karthik --- drivers/staging/comedi/comedi_fops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 3f70e5dfac39..9cdc1e8a022d 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2956,7 +2956,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, cmd->scan_end_arg = v32.scan_end_arg; cmd->stop_src = v32.stop_src; cmd->stop_arg = v32.stop_arg; - cmd->chanlist = compat_ptr(v32.chanlist); + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); cmd->chanlist_len = v32.chanlist_len; cmd->data = compat_ptr(v32.data); cmd->data_len = v32.data_len; @@ -2983,7 +2983,7 @@ static int put_compat_cmd(struct comedi32_cmd_struct __user *cmd32, v32.stop_src = cmd->stop_src; v32.stop_arg = cmd->stop_arg; /* Assume chanlist pointer is unchanged. */ - v32.chanlist = ptr_to_compat(cmd->chanlist); + v32.chanlist = ptr_to_compat((unsigned int __user *)cmd->chanlist); v32.chanlist_len = cmd->chanlist_len; v32.data = ptr_to_compat(cmd->data); v32.data_len = cmd->data_len; Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: comedi_fops.c: changed type in assignment to unsigned int *
On 15/07/2020 05:48, B K Karthik wrote: fixed a sparse warning by changing the type in assignment from void [noderef] __user * to unsigned int * (different address space) Signed-off-by: B K Karthik --- drivers/staging/comedi/comedi_fops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 3f70e5dfac39..4cc012e231b7 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2956,7 +2956,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, cmd->scan_end_arg = v32.scan_end_arg; cmd->stop_src = v32.stop_src; cmd->stop_arg = v32.stop_arg; - cmd->chanlist = compat_ptr(v32.chanlist); + cmd->chanlist = (unsigned int *) compat_ptr(v32.chanlist); That should be: cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); cmd->chanlist_len = v32.chanlist_len; cmd->data = compat_ptr(v32.data); cmd->data_len = v32.data_len; A reverse cast is required in put_compat_cmd(): v32.chanlist = ptr_to_compat((unsigned int __user *)cmd->chanlist); Those changes will get rid of the sparse warnings. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH v2] staging: comedi: s626: Remove pci-dma-compat wrapper APIs.
On 13/07/2020 15:32, Suraj Upadhyay wrote: The legacy API wrappers in include/linux/pci-dma-compat.h should go away as it creates unnecessary midlayering for include/linux/dma-mapping.h APIs, instead use dma-mapping.h APIs directly. The patch has been generated with the coccinelle script below and compile-tested. - PCI_DMA_BIDIRECTIONAL + DMA_BIDIRECTIONAL - PCI_DMA_TODEVICE + DMA_TO_DEVICE - PCI_DMA_FROMDEVICE + DMA_FROM_DEVICE - PCI_DMA_NONE + DMA_NONE @@ expression E1, E2, E3; @@ - pci_alloc_consistent(E1, E2, E3) + dma_alloc_coherent(>dev, E2, E3, GFP_ATOMIC) @@ expression E1, E2, E3; @@ - pci_zalloc_consistent(E1, E2, E3) + dma_alloc_coherent(>dev, E2, E3, GFP_ATOMIC) @@ expression E1, E2, E3, E4; @@ - pci_free_consistent(E1, E2, E3, E4) + dma_free_coherent(>dev, E2, E3, E4) @@ expression E1, E2, E3, E4; @@ - pci_map_single(E1, E2, E3, E4) + dma_map_single(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_unmap_single(E1, E2, E3, E4) + dma_unmap_single(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4, E5; @@ - pci_map_page(E1, E2, E3, E4, E5) + dma_map_page(>dev, E2, E3, E4, (enum dma_data_direction)E5) @@ expression E1, E2, E3, E4; @@ - pci_unmap_page(E1, E2, E3, E4) + dma_unmap_page(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_map_sg(E1, E2, E3, E4) + dma_map_sg(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_unmap_sg(E1, E2, E3, E4) + dma_unmap_sg(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_dma_sync_single_for_cpu(E1, E2, E3, E4) + dma_sync_single_for_cpu(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_dma_sync_single_for_device(E1, E2, E3, E4) + dma_sync_single_for_device(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_dma_sync_sg_for_cpu(E1, E2, E3, E4) + dma_sync_sg_for_cpu(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2, E3, E4; @@ - pci_dma_sync_sg_for_device(E1, E2, E3, E4) + dma_sync_sg_for_device(>dev, E2, E3, (enum dma_data_direction)E4) @@ expression E1, E2; @@ - pci_dma_mapping_error(E1, E2) + dma_mapping_error(>dev, E2) @@ expression E1, E2; @@ - pci_set_consistent_dma_mask(E1, E2) + dma_set_coherent_mask(>dev, E2) @@ expression E1, E2; @@ - pci_set_dma_mask(E1, E2) + dma_set_mask(>dev, E2) Signed-off-by: Suraj Upadhyay --- Changes: v2: Converted the GFP_ATOMIC flag to GFP_KERNEL to suit for the context. On reviewer's advise. drivers/staging/comedi/drivers/s626.c | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 084a8e7b9fc2..e7aba937d896 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -2130,13 +2130,15 @@ static int s626_allocate_dma_buffers(struct comedi_device *dev) void *addr; dma_addr_t appdma; - addr = pci_alloc_consistent(pcidev, S626_DMABUF_SIZE, ); + addr = dma_alloc_coherent(>dev, S626_DMABUF_SIZE, , + GFP_KERNEL); if (!addr) return -ENOMEM; devpriv->ana_buf.logical_base = addr; devpriv->ana_buf.physical_base = appdma; - addr = pci_alloc_consistent(pcidev, S626_DMABUF_SIZE, ); + addr = dma_alloc_coherent(>dev, S626_DMABUF_SIZE, , + GFP_KERNEL); if (!addr) return -ENOMEM; devpriv->rps_buf.logical_base = addr; @@ -2154,13 +2156,13 @@ static void s626_free_dma_buffers(struct comedi_device *dev) return; if (devpriv->rps_buf.logical_base) - pci_free_consistent(pcidev, S626_DMABUF_SIZE, - devpriv->rps_buf.logical_base, - devpriv->rps_buf.physical_base); + dma_free_coherent(>dev, S626_DMABUF_SIZE, + devpriv->rps_buf.logical_base, + devpriv->rps_buf.physical_base); if (devpriv->ana_buf.logical_base) - pci_free_consistent(pcidev, S626_DMABUF_SIZE, - devpriv->ana_buf.logical_base, - devpriv->ana_buf.physical_base); + dma_free_coherent(>dev, S626_DMABUF_SIZE, + devpriv->ana_buf.logical_base, + devpriv->ana_buf.physical_base); } static int s626_initialize(struct comedi_device *dev) Looks good to me. Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: s626: Remove pci-dma-compat wrapper APIs.
On 11/07/2020 14:38, Christophe JAILLET wrote: Le 11/07/2020 à 14:35, Suraj Upadhyay a écrit : The legacy API wrappers in include/linux/pci-dma-compat.h should go away as it creates unnecessary midlayering for include/linux/dma-mapping.h APIs, instead use dma-mapping.h APIs directly. The patch has been generated with the coccinelle script below and compile-tested. [...] @@ expression E1, E2, E3; @@ - pci_alloc_consistent(E1, E2, E3) + dma_alloc_coherent(>dev, E2, E3, GFP_ATOMIC) @@ expression E1, E2, E3; @@ - pci_zalloc_consistent(E1, E2, E3) + dma_alloc_coherent(>dev, E2, E3, GFP_ATOMIC) This is the tricky part of this kind of cleanup, see below. GFP_ATOMIC can't be wrong because it is was exactly what was done with the pci_ function. However, most of the time, it can safely be replaced by GFP_KERNEL which gives more opportunities to the memory allocator. [...] diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c index 084a8e7b9fc2..c159416662fd 100644 --- a/drivers/staging/comedi/drivers/s626.c +++ b/drivers/staging/comedi/drivers/s626.c @@ -2130,13 +2130,15 @@ static int s626_allocate_dma_buffers(struct comedi_device *dev) void *addr; dma_addr_t appdma; - addr = pci_alloc_consistent(pcidev, S626_DMABUF_SIZE, ); + addr = dma_alloc_coherent(>dev, S626_DMABUF_SIZE, , + GFP_ATOMIC); if (!addr) return -ENOMEM; devpriv->ana_buf.logical_base = addr; devpriv->ana_buf.physical_base = appdma; - addr = pci_alloc_consistent(pcidev, S626_DMABUF_SIZE, ); + addr = dma_alloc_coherent(>dev, S626_DMABUF_SIZE, , + GFP_ATOMIC); if (!addr) return -ENOMEM; devpriv->rps_buf.logical_base = addr; 's626_allocate_dma_buffers()' is only called from 's626_auto_attach()'. In this function, around the call to 's626_allocate_dma_buffers()', you can see: - a few lines before, a call to 'comedi_alloc_devpriv()' - a few lines after, a call to 'comedi_alloc_subdevices()' These 2 functions make some memory allocation using GFP_KERNEL. So it is likely that using GFP_KERNEL in your proposal is also valid. Indeed, GFP_KERNEL is perfectly fine here. It could be done as a follow-up patch, or done in a v2 of this patch with a mention in the patch description. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] Staging: comedi: driver: Remove condition with no effect
On 13/07/2020 14:34, Greg KH wrote:
On Sun, Jul 12, 2020 at 12:36:28PM +0530, Saurav Girepunje wrote:
Remove below warning in das1800.c
WARNING: possible condition with no effect (if == else)
Signed-off-by: Saurav Girepunje
---
drivers/staging/comedi/drivers/das1800.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/das1800.c
b/drivers/staging/comedi/drivers/das1800.c
index f16aa7e9f4f3..7ab72e83d3d0 100644
--- a/drivers/staging/comedi/drivers/das1800.c
+++ b/drivers/staging/comedi/drivers/das1800.c
@@ -1299,12 +1299,6 @@ static int das1800_attach(struct comedi_device *dev,
outb(DAC(i), dev->iobase + DAS1800_SELECT);
outw(0, dev->iobase + DAS1800_DAC);
}
- } else if (board->id == DAS1800_ID_AO) {
- /*
-* 'ao' boards have waveform analog outputs that are not
-* currently supported.
-*/
- s->type = COMEDI_SUBD_UNUSED;
What gave that warning? The comment should show you why this is good to
keep as-is, right?
One option is to move the comment into the '} else {' part that follows
this part.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCHES] uaccess comedi compat
On 29/05/2020 01:34, Al Viro wrote:
The way comedi compat ioctls are done is wrong.
Instead of having ->compat_ioctl() copying the 32bit
stuff in, then passing the kernel copies to helpers shared
with native ->ioctl() and doing copyout with conversion if
needed, it's playing silly buggers with creating a 64bit
copy on user stack, then calling native ioctl (which copies
that copy into the kernel), then fetching it from user stack,
converting to 32bit variant and copying that to user.
Extra headache for no good reason. And the single
largest remaining pile of __put_user()/__get_user() this side
of arch/*. IMO compat_alloc_user_space() should die...
NOTE: this is only compile-tested - I simply don't
have the hardware in question.
Anyway, the branch lives in #uaccess.comedi, based
at v5.7-rc1
Al Viro (10):
comedi: move compat ioctl handling to native fops
comedi: get rid of indirection via translated_ioctl()
comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO
compat
comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO
compat
comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat
comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST
compat
comedi: lift copy_from_user() into callers of __comedi_get_user_cmd()
comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller
comedi: do_cmd_ioctl(): lift copyin/copyout into the caller
comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST}
compat
There is a bug in patch 05. Patch 10 doesn't seem to have been sent yet
(I didn't receive it and I can't see it in the thread in the LKML
archives). I've signed off on 01-04, 06-09.
These should be Cc'd to Greg KH and to de...@driverdev.osuosl.org.
Cheers,
Ian
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 09/10] comedi: do_cmd_ioctl(): lift copyin/copyout into the caller
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 48 ++-- 1 file changed, 24 insertions(+), 24 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 138 --- 1 file changed, 48 insertions(+), 90 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Just take copy_from_user() out of do_chaninfo_ioctl() into the caller and have compat_chaninfo() build a native version and pass it to do_chaninfo_ioctl() directly. Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 68 1 file changed, 30 insertions(+), 38 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 01/10] comedi: move compat ioctl handling to native fops
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro mechanical move Signed-off-by: Al Viro --- drivers/staging/comedi/Makefile | 1 - drivers/staging/comedi/comedi_compat32.c | 455 --- drivers/staging/comedi/comedi_compat32.h | 28 -- drivers/staging/comedi/comedi_fops.c | 451 +- 4 files changed, 448 insertions(+), 487 deletions(-) delete mode 100644 drivers/staging/comedi/comedi_compat32.c delete mode 100644 drivers/staging/comedi/comedi_compat32.h Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 02/10] comedi: get rid of indirection via translated_ioctl()
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 26 -- 1 file changed, 8 insertions(+), 18 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Just take copy_from_user() out of do_rangeing_ioctl() into the caller and have compat_rangeinfo() build a native version and pass it to do_rangeinfo_ioctl() directly. Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 43 ++-- drivers/staging/comedi/comedi_internal.h | 2 +- drivers/staging/comedi/range.c | 17 ++--- 3 files changed, 27 insertions(+), 35 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 45 ++-- 1 file changed, 22 insertions(+), 23 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd()
On 29/05/2020 01:35, Al Viro wrote: From: Al Viro Signed-off-by: Al Viro --- drivers/staging/comedi/comedi_fops.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) Signed-off-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat
On 29/05/2020 01:35, Al Viro wrote:
From: Al Viro
Just take copy_from_user() out of do_insn_ioctl() into the caller and
have compat_insn() build a native version and pass it to do_insn_ioctl()
directly.
One difference from the previous commits is that the helper used to
convert 32bit variant to native has two users - compat_insn() and
compat_insnlist(). The latter will be converted in next commit;
for now we simply split the helper in two variants - "userland 32bit
to kernel native" and "userland 32bit to userland native". The latter
is renamed old get_compat_insn(); it will be gone in the next commit.
Signed-off-by: Al Viro
---
drivers/staging/comedi/comedi_fops.c | 73 +++-
1 file changed, 46 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index d96dc85d8a98..ae0067ab5ead 100644
[snip]
@@ -2244,10 +2241,13 @@ static long comedi_unlocked_ioctl(struct file *file,
unsigned int cmd,
(struct comedi_insnlist __user *)arg,
file);
break;
- case COMEDI_INSN:
- rc = do_insn_ioctl(dev, (struct comedi_insn __user *)arg,
- file);
+ case COMEDI_INSN: {
+ struct comedi_insn insn;
+ if (copy_from_user(, (void __user *)arg, sizeof(insn)))
+ rc = -EFAULT;
Missing an 'else' here:
+ rc = do_insn_ioctl(dev, , file);
break;
+ }
case COMEDI_POLL:
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH -next] staging: comedi: remove unused variable 'route_table_size'
On 23/10/2019 08:52, YueHaibing wrote: drivers/staging/comedi/drivers/ni_routes.c:52:21: warning: route_table_size defined but not used [-Wunused-const-variable=] It is never used since introduction. Signed-off-by: YueHaibing --- drivers/staging/comedi/drivers/ni_routes.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/ni_routes.c b/drivers/staging/comedi/drivers/ni_routes.c index eb61494..673d732 100644 --- a/drivers/staging/comedi/drivers/ni_routes.c +++ b/drivers/staging/comedi/drivers/ni_routes.c @@ -49,8 +49,6 @@ /* Helper for accessing data. */ #define RVi(table, src, dest) ((table)[(dest) * NI_NUM_NAMES + (src)]) -static const size_t route_table_size = NI_NUM_NAMES * NI_NUM_NAMES; - /* * Find the proper route_values and ni_device_routes tables for this particular * device. Looks good, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH -next] staging: comedi: dt2814: remove set but not used variables 'data'
On 23/10/2019 08:48, YueHaibing wrote:
drivers/staging/comedi/drivers/dt2814.c:193:6:
warning: variable data set but not used [-Wunused-but-set-variable]
It is never used, so can be removed.
Signed-off-by: YueHaibing
---
drivers/staging/comedi/drivers/dt2814.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2814.c
b/drivers/staging/comedi/drivers/dt2814.c
index d2c7157..e7c6787 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -186,21 +186,17 @@ static int dt2814_ai_cmd(struct comedi_device *dev,
struct comedi_subdevice *s)
static irqreturn_t dt2814_interrupt(int irq, void *d)
{
- int lo, hi;
struct comedi_device *dev = d;
struct dt2814_private *devpriv = dev->private;
struct comedi_subdevice *s = dev->read_subdev;
- int data;
if (!dev->attached) {
dev_err(dev->class_dev, "spurious interrupt\n");
return IRQ_HANDLED;
}
- hi = inb(dev->iobase + DT2814_DATA);
- lo = inb(dev->iobase + DT2814_DATA);
-
- data = (hi << 4) | (lo >> 4);
+ inb(dev->iobase + DT2814_DATA);
+ inb(dev->iobase + DT2814_DATA);
if (!(--devpriv->ntrig)) {
int i;
Those variables are currently unused due to a bug that I need to fix.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: drivers: prevent memory leak
On 17/09/2019 07:33, Dan Carpenter wrote:
On Mon, Sep 16, 2019 at 09:41:43PM -0500, Navid Emamdoost wrote:
In das1800_attach, the buffer allocated via kmalloc_array needs to be
released if an error happens.
Signed-off-by: Navid Emamdoost
Commedit calls ->detach() if the ->attach() fails so this patch would
lead to a double free. See comedi_device_attach():
drivers/staging/comedi/drivers.c
983 }
984 if (!driv->attach) {
985 /* driver does not support manual configuration */
986 dev_warn(dev->class_dev,
987 "driver '%s' does not support attach using
comedi_config\n",
988 driv->driver_name);
989 module_put(driv->module);
990 ret = -EIO;
991 goto out;
992 }
993 dev->driver = driv;
994 dev->board_name = dev->board_ptr ? *(const char
**)dev->board_ptr
995 : dev->driver->driver_name;
996 ret = driv->attach(dev, it);
^
997 if (ret >= 0)
998 ret = comedi_device_postconfig(dev);
999 if (ret < 0) {
1000 comedi_device_detach(dev);
^
1001 module_put(driv->module);
1002 }
1003 /* On success, the driver module count has been incremented. */
Yes, everything should be freed properly by comedi_device_detach().
From comedi_device_detach(), some of the stuff is freed by
dev->driver->detach(), and the remainder is freed by
comedi_device_detach_cleanup().
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: usbduxsigma: remove redundant assignment to variable fx2delay
On 15/08/2019 11:53, Colin King wrote:
From: Colin Ian King
Variable fx2delay is being initialized with a value that is never read
and fx2delay is being re-assigned a little later on. The assignment is
redundant and hence can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King
---
drivers/staging/comedi/drivers/usbduxsigma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c
b/drivers/staging/comedi/drivers/usbduxsigma.c
index 3cc40d2544be..54d7605e909f 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1074,7 +1074,7 @@ static int usbduxsigma_pwm_period(struct comedi_device
*dev,
unsigned int period)
{
struct usbduxsigma_private *devpriv = dev->private;
- int fx2delay = 255;
+ int fx2delay;
if (period < MIN_PWM_PERIOD)
return -EAGAIN;
Looks fine, thanks!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[PATCH] staging: comedi: dt3000: Fix signed integer overflow 'divider * base'
In `dt3k_ns_to_timer()` the following lines near the end of the function result in a signed integer overflow: prescale = 15; base = timer_base * (1 << prescale); divider = 65535; *nanosec = divider * base; (`divider`, `base` and `prescale` are type `int`, `timer_base` and `*nanosec` are type `unsigned int`. The value of `timer_base` will be either 50 or 100.) The main reason for the overflow is that the calculation for `base` is completely wrong. It should be: base = timer_base * (prescale + 1); which matches an earlier instance of this calculation in the same function. Reported-by: David Binderman Cc: Signed-off-by: Ian Abbott --- N.B. Greg: The original report suggested an actual build error, so may be prudent to queue this on your 'staging-linus' queue. drivers/staging/comedi/drivers/dt3000.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/dt3000.c b/drivers/staging/comedi/drivers/dt3000.c index 2edf3ee91300..4ad176fc14ad 100644 --- a/drivers/staging/comedi/drivers/dt3000.c +++ b/drivers/staging/comedi/drivers/dt3000.c @@ -368,7 +368,7 @@ static int dt3k_ns_to_timer(unsigned int timer_base, unsigned int *nanosec, } prescale = 15; - base = timer_base * (1 << prescale); + base = timer_base * (prescale + 1); divider = 65535; *nanosec = divider * base; return (prescale << 16) | (divider); -- 2.20.1
Re: linux-5.3-rc4/drivers/staging/comedi/drivers/dt3000.c:373: (error) Signed integer overflow for expression 'divider*base'
On 12/08/2019 08:37, David Binderman wrote: Hello there, Source code is prescale = 15; base = timer_base * (1 << prescale); divider = 65535; *nanosec = divider * base; timer_base seems to be 500 or 100. nanosec is a pointer to int, so it can only hold about 2,000,000,000 nanoseconds, or about 2 seconds. Thanks for the report. I couldn't reproduce the error with the compiler version I was using, but I can see that it is likely to result in an arithmetic overflow. I think the main problem is that the line (in (dt3k_ns_to_timer()): base = timer_base * (1 << prescale); should be: base = timer_base * (prescale + 1); which matches the earlier instance of this calculation in the same function. In practice, these lines of code should never be reached due to earlier range checks in dt3k_ai_cmdtest(). Suggest rework code to use longs. It wouldn't do any harm to change the `int` variables to `unsigned int`. Regards David Binderman time-_Base seems to be 50 or 100. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH][next] staging: comedi: usbdux: remove redundant initialization of fx2delay
On 17/06/2019 14:03, Colin King wrote:
From: Colin Ian King
Variable fx2delay is being initialized to a value that is never read
and is being re-assigned a few statements later. The initialization
is redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King
---
drivers/staging/comedi/drivers/usbdux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/usbdux.c
b/drivers/staging/comedi/drivers/usbdux.c
index b8f54b7fb34a..0350f303d557 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1226,7 +1226,7 @@ static int usbdux_pwm_period(struct comedi_device *dev,
unsigned int period)
{
struct usbdux_private *devpriv = dev->private;
- int fx2delay = 255;
+ int fx2delay;
if (period < MIN_PWM_PERIOD)
return -EAGAIN;
Looks good, thanks!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 12/16] staging/comedi: mark as broken
On 14/06/2019 16:34, Christoph Hellwig wrote: On Fri, Jun 14, 2019 at 05:30:32PM +0200, Greg KH wrote: On Fri, Jun 14, 2019 at 04:48:57PM +0200, Christoph Hellwig wrote: On Fri, Jun 14, 2019 at 04:02:39PM +0200, Greg KH wrote: Perhaps a hint as to how we can fix this up? This is the first time I've heard of the comedi code not handling dma properly. It can be fixed by: a) never calling virt_to_page (or vmalloc_to_page for that matter) on dma allocation b) never remapping dma allocation with conflicting cache modes (no remapping should be doable after a) anyway). Ok, fair enough, have any pointers of drivers/core code that does this correctly? I can put it on my todo list, but might take a week or so... Just about everyone else. They just need to remove the vmap and either do one large allocation, or live with the fact that they need helpers to access multiple array elements instead of one net vmap, which most of the users already seem to do anyway, with just a few using the vmap (which might explain why we didn't see blowups yet). Avoiding the vmap in comedi should be do-able as it already has other means to get at the buffer pages. When comedi makes the buffer from DMA coherent memory, it currently allocates it as a series of page-sized chunks. That cannot be mmap'ed in one go with dma_mmap_coherent(), so I see the following solutions. 1. Change the buffer allocation to allocate a single chunk of DMA coherent memory and use dma_mmap_coherent() to mmap it. 2. Call dma_mmap_coherent() in a loop, adjusting vma->vm_start and vma->vm_end for each iteration (vma->vm_pgoff will be 0), and restoring the vma->vm_start and vma->vm_end at the end. I'm not sure if 2 is a legal option. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: Remove variable runflags
On 30/05/2019 21:51, Nishka Dasgupta wrote:
Remove variable runflags and use its value directly. Issue found with
checkpatch.
Signed-off-by: Nishka Dasgupta
---
drivers/staging/comedi/comedi_fops.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index f6d1287c7b83..b84ee9293903 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -676,16 +676,12 @@ EXPORT_SYMBOL_GPL(comedi_is_subdevice_running);
static bool __comedi_is_subdevice_running(struct comedi_subdevice *s)
{
- unsigned int runflags = __comedi_get_subdevice_runflags(s);
-
- return comedi_is_runflags_running(runflags);
+ return comedi_is_runflags_running(__comedi_get_subdevice_runflags(s));
}
bool comedi_can_auto_free_spriv(struct comedi_subdevice *s)
{
- unsigned int runflags = __comedi_get_subdevice_runflags(s);
-
- return runflags & COMEDI_SRF_FREE_SPRIV;
+ return __comedi_get_subdevice_runflags(s) & COMEDI_SRF_FREE_SPRIV;
}
/**
I couldn't reproduce this checkpatch issue, even with '--subjective'.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[PATCH v2] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API
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, fall back to using the comedi "class" device
pointed to by the `class_dev` member if that is non-NULL. In that case,
make it "DMA-capable" with a coherent DMA mask set to the ISA bus limit
of 16MB (24 bits).
Signed-off-by: Ian Abbott
---
v2: Use the comedi "class" device instead of a static fallback device.
---
drivers/staging/comedi/drivers/comedi_isadma.c | 17 +++--
drivers/staging/comedi/drivers/comedi_isadma.h | 3 +++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c
b/drivers/staging/comedi/drivers/comedi_isadma.c
index b77dc8d5d3ff..c729094298c2 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.c
+++ b/drivers/staging/comedi/drivers/comedi_isadma.c
@@ -172,6 +172,19 @@ struct comedi_isadma *comedi_isadma_alloc(struct
comedi_device *dev,
goto no_dma;
dma->desc = desc;
dma->n_desc = n_desc;
+ if (dev->hw_dev) {
+ dma->dev = dev->hw_dev;
+ } else {
+ /* Fall back to using the "class" device. */
+ if (!dev->class_dev)
+ goto no_dma;
+ /* Need 24-bit mask for ISA DMA. */
+ if (dma_coerce_mask_and_coherent(dev->class_dev,
+DMA_BIT_MASK(24))) {
+ goto no_dma;
+ }
+ dma->dev = dev->class_dev;
+ }
dma_chans[0] = dma_chan1;
if (dma_chan2 == 0 || dma_chan2 == dma_chan1)
@@ -192,7 +205,7 @@ struct comedi_isadma *comedi_isadma_alloc(struct
comedi_device *dev,
desc = >desc[i];
desc->chan = dma_chans[i];
desc->maxsize = maxsize;
- desc->virt_addr = dma_alloc_coherent(NULL, desc->maxsize,
+ desc->virt_addr = dma_alloc_coherent(dma->dev, desc->maxsize,
>hw_addr,
GFP_KERNEL);
if (!desc->virt_addr)
@@ -224,7 +237,7 @@ void comedi_isadma_free(struct comedi_isadma *dma)
for (i = 0; i < dma->n_desc; i++) {
desc = >desc[i];
if (desc->virt_addr)
- dma_free_coherent(NULL, desc->maxsize,
+ dma_free_coherent(dma->dev, desc->maxsize,
desc->virt_addr,
desc->hw_addr);
}
diff --git a/drivers/staging/comedi/drivers/comedi_isadma.h
b/drivers/staging/comedi/drivers/comedi_isadma.h
index 2bd1329d727f..9d2b12db7e6e 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.h
+++ b/drivers/staging/comedi/drivers/comedi_isadma.h
@@ -10,6 +10,7 @@
#include
struct comedi_device;
+struct device;
/*
* These are used to avoid issues when and the DMA_MODE_
@@ -38,6 +39,7 @@ struct comedi_isadma_desc {
/**
* struct comedi_isadma - ISA DMA data
+ * @dev: device to allocate non-coherent memory for
* @desc: cookie for each DMA buffer
* @n_desc:the number of cookies
* @cur_dma: the current cookie in use
@@ -45,6 +47,7 @@ struct comedi_isadma_desc {
* @chan2: the second DMA channel requested
*/
struct comedi_isadma {
+ struct device *dev;
struct comedi_isadma_desc *desc;
int n_desc;
int cur_dma;
--
2.20.1
Re: [PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API
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 = _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.
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).
thanks,
greg k-h
Thanks for the review,
Ian Abbott.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API
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 = _dev.coherent_dma_mask,
+};
+
/**
* comedi_isadma_program - program and enable an ISA DMA transfer
* @desc: the ISA DMA cookie to program and enable
@@ -172,6 +182,7 @@ struct comedi_isadma *comedi_isadma_alloc(struct
comedi_device *dev,
goto no_dma;
dma->desc = desc;
dma->n_desc = n_desc;
+ dma->dev = dev->hw_dev ? dev->hw_dev : _dev;
dma_chans[0] = dma_chan1;
if (dma_chan2 == 0 || dma_chan2 == dma_chan1)
@@ -192,7 +203,7 @@ struct comedi_isadma *comedi_isadma_alloc(struct
comedi_device *dev,
desc = >desc[i];
desc->chan = dma_chans[i];
desc->maxsize = maxsize;
- desc->virt_addr = dma_alloc_coherent(NULL, desc->maxsize,
+ desc->virt_addr = dma_alloc_coherent(dma->dev, desc->maxsize,
>hw_addr,
GFP_KERNEL);
if (!desc->virt_addr)
@@ -224,7 +235,7 @@ void comedi_isadma_free(struct comedi_isadma *dma)
for (i = 0; i < dma->n_desc; i++) {
desc = >desc[i];
if (desc->virt_addr)
- dma_free_coherent(NULL, desc->maxsize,
+ dma_free_coherent(dma->dev, desc->maxsize,
desc->virt_addr,
desc->hw_addr);
}
diff --git a/drivers/staging/comedi/drivers/comedi_isadma.h
b/drivers/staging/comedi/drivers/comedi_isadma.h
index 2bd1329d727f..9d2b12db7e6e 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.h
+++ b/drivers/staging/comedi/drivers/comedi_isadma.h
@@ -10,6 +10,7 @@
#include
struct comedi_device;
+struct device;
/*
* These are used to avoid issues when and the DMA_MODE_
@@ -38,6 +39,7 @@ struct comedi_isadma_desc {
/**
* struct comedi_isadma - ISA DMA data
+ * @dev: device to allocate non-coherent memory for
* @desc: cookie for each DMA buffer
* @n_desc:the number of cookies
* @cur_dma: the current cookie in use
@@ -45,6 +47,7 @@ struct comedi_isadma_desc {
* @chan2: the second DMA channel requested
*/
struct comedi_isadma {
+ struct device *dev;
struct comedi_isadma_desc *desc;
int n_desc;
int cur_dma;
--
2.20.1
[PATCH] fuse: Add ioctl flag for x32 compat ioctl
Currently, a CUSE server running on a 64-bit kernel can tell when an
ioctl request comes from a process running a 32-bit ABI, but cannot tell
whether the requesting process is using legacy IA32 emulation or x32
ABI. In particular, the server does not know the size of the client
process's `time_t` type.
For 64-bit kernels, the `FUSE_IOCTL_COMPAT` and `FUSE_IOCTL_32BIT` flags
are currently set in the ioctl input request (`struct fuse_ioctl_in`
member `flags`) for a 32-bit requesting process. This patch defines a
new flag `FUSE_IOCTL_COMPAT_X32` and sets it if the 32-bit requesting
process is using the x32 ABI. This allows the server process to
distinguish between requests from requesting client processes using IA32
emulation or the x32 ABI and so infer the size of the client process's
`time_t` type and any other IA32/x32 differences.
Signed-off-by: Ian Abbott
Cc: Miklos Szeredi
---
This is based on an earlier patch titled "fuse: Add ioctl flag for
compat ioctl with 64-bit time_t" that focussed specifically on `time_t`
compatibility. At the insistence of Miklos, I have changed it to focus
on x32 compatibility. -- Ian Abbott
---
fs/fuse/file.c| 7 ++-
include/uapi/linux/fuse.h | 2 ++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 06096b60f1df..44d52c54dd75 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2576,8 +2576,13 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd,
unsigned long arg,
#if BITS_PER_LONG == 32
inarg.flags |= FUSE_IOCTL_32BIT;
#else
- if (flags & FUSE_IOCTL_COMPAT)
+ if (flags & FUSE_IOCTL_COMPAT) {
inarg.flags |= FUSE_IOCTL_32BIT;
+#ifdef CONFIG_X86_X32
+ if (in_x32_syscall())
+ inarg.flags |= FUSE_IOCTL_COMPAT_X32;
+#endif
+ }
#endif
/* assume all the iovs returned by client always fits in a page */
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2ac598614a8f..c8ba8230639e 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -335,6 +335,7 @@ struct fuse_file_lock {
* FUSE_IOCTL_RETRY: retry with new iovecs
* FUSE_IOCTL_32BIT: 32bit ioctl
* FUSE_IOCTL_DIR: is a directory
+ * FUSE_IOCTL_COMPAT_X32: x32 compat ioctl on 64bit machine (64bit time_t)
*
* FUSE_IOCTL_MAX_IOV: maximum of in_iovecs + out_iovecs
*/
@@ -343,6 +344,7 @@ struct fuse_file_lock {
#define FUSE_IOCTL_RETRY (1 << 2)
#define FUSE_IOCTL_32BIT (1 << 3)
#define FUSE_IOCTL_DIR (1 << 4)
+#define FUSE_IOCTL_COMPAT_X32 (1 << 5)
#define FUSE_IOCTL_MAX_IOV 256
--
2.20.1
Re: [PATCH] fuse: Add ioctl flag for compat ioctl with 64-bit time_t
On 24/04/2019 11:52, Miklos Szeredi wrote: On Tue, Apr 23, 2019 at 5:14 PM Ian Abbott wrote: On 23/04/2019 13:55, Miklos Szeredi wrote: On Fri, Mar 1, 2019 at 6:08 PM Ian Abbott wrote: Currently, a CUSE server running on a 64-bit kernel can tell when an ioctl request comes from a process running a 32-bit ABI, but cannot tell whether the requesting process is using legacy IA32 emulation or x32 ABI, for example. In particular, the server does not know the size of the client process's `time_t` type. For 64-bit kernels, the `FUSE_IOCTL_COMPAT` and `FUSE_IOCTL_32BIT` flags are currently set in the ioctl input request (`struct fuse_ioctl_in` member `flags`) for a 32-bit requesting process. This patch defines a new flag `FUSE_IOCTL_COMPAT_64TIME` and sets it if the 32-bit requesting process (running on a 64-bit kernel) uses a 64-bit `time_t` type. Hi, Thanks for the patch. I think it should rather use in_x32_syscall() helper and follow that naming because there's apparently at least one example in xfs of a non-time_t related ioctl that varies between the x32 vs ia32. Hi Miklos, It is conceivable that COMPAT_USE_64BIT_TIME could be true for some other arch/ABI (although currently it is only ever set for x86/x32). Should it have separate flags for "compat 64-bit time" and "compat x32" (even though that is currently redundant)? No, it should just be a single flag, something like FUSE_IOCTL_COMPAT_X32 and the documentation should say that it implies 64bit time values. Okay, since the subject line, description and code will change, I'll submit the new patch as a new patch, rather than a "v2". (I'll mention its history below the scissors line.) Please drop this patch. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] fuse: Add ioctl flag for compat ioctl with 64-bit time_t
On 23/04/2019 13:55, Miklos Szeredi wrote:
On Fri, Mar 1, 2019 at 6:08 PM Ian Abbott wrote:
Currently, a CUSE server running on a 64-bit kernel can tell when an
ioctl request comes from a process running a 32-bit ABI, but cannot tell
whether the requesting process is using legacy IA32 emulation or x32
ABI, for example. In particular, the server does not know the size of
the client process's `time_t` type.
For 64-bit kernels, the `FUSE_IOCTL_COMPAT` and `FUSE_IOCTL_32BIT` flags
are currently set in the ioctl input request (`struct fuse_ioctl_in`
member `flags`) for a 32-bit requesting process. This patch defines a
new flag `FUSE_IOCTL_COMPAT_64TIME` and sets it if the 32-bit requesting
process (running on a 64-bit kernel) uses a 64-bit `time_t` type.
Hi,
Thanks for the patch.
I think it should rather use in_x32_syscall() helper and follow that
naming because there's apparently at least one example in xfs of a
non-time_t related ioctl that varies between the x32 vs ia32.
Hi Miklos,
It is conceivable that COMPAT_USE_64BIT_TIME could be true for some
other arch/ABI (although currently it is only ever set for x86/x32).
Should it have separate flags for "compat 64-bit time" and "compat x32"
(even though that is currently redundant)?
Kind regards,
Ian.
Thanks,
Miklos
Signed-off-by: Ian Abbott
---
fs/fuse/file.c| 5 -
include/uapi/linux/fuse.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 06096b60f1df..9777e7a19889 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2576,8 +2576,11 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd,
unsigned long arg,
#if BITS_PER_LONG == 32
inarg.flags |= FUSE_IOCTL_32BIT;
#else
- if (flags & FUSE_IOCTL_COMPAT)
+ if (flags & FUSE_IOCTL_COMPAT) {
inarg.flags |= FUSE_IOCTL_32BIT;
+ if (COMPAT_USE_64BIT_TIME)
+ inarg.flags |= FUSE_IOCTL_COMPAT_64TIME;
+ }
#endif
/* assume all the iovs returned by client always fits in a page */
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2ac598614a8f..1f4a71486601 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -335,6 +335,7 @@ struct fuse_file_lock {
* FUSE_IOCTL_RETRY: retry with new iovecs
* FUSE_IOCTL_32BIT: 32bit ioctl
* FUSE_IOCTL_DIR: is a directory
+ * FUSE_IOCTL_COMPAT_64TIME: 32bit compat ioctl with 64bit time_t
*
* FUSE_IOCTL_MAX_IOV: maximum of in_iovecs + out_iovecs
*/
@@ -343,6 +344,7 @@ struct fuse_file_lock {
#define FUSE_IOCTL_RETRY (1 << 2)
#define FUSE_IOCTL_32BIT (1 << 3)
#define FUSE_IOCTL_DIR (1 << 4)
+#define FUSE_IOCTL_COMPAT_64TIME (1 << 5)
#define FUSE_IOCTL_MAX_IOV 256
--
2.20.1
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: use help instead of ---help--- in Kconfig
On 16/04/2019 18:08, Moses Christopher wrote: - Resolve the following warning from the Kconfig, "WARNING: prefer 'help' over '---help---' for new help texts" Signed-off-by: Moses Christopher --- drivers/staging/comedi/Kconfig | 254 - 1 file changed, 127 insertions(+), 127 deletions(-) Looks good, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: adv_pci1710: fix spelling mistake: "droput" -> "dropout"
On 15/04/2019 18:49, Colin King wrote:
From: Colin Ian King
There is a spelling mistake in a dev_error message. Fix it.
Signed-off-by: Colin Ian King
Looks good, thanks!
Reviewed-by: Ian Abbott
---
drivers/staging/comedi/drivers/adv_pci1710.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c
b/drivers/staging/comedi/drivers/adv_pci1710.c
index 6a93b04f1fdf..dbff0f7e7cf5 100644
--- a/drivers/staging/comedi/drivers/adv_pci1710.c
+++ b/drivers/staging/comedi/drivers/adv_pci1710.c
@@ -317,7 +317,7 @@ static int pci1710_ai_read_sample(struct comedi_device *dev,
chan = sample >> 12;
if (chan != devpriv->act_chanlist[cur_chan]) {
dev_err(dev->class_dev,
- "A/D data droput: received from channel %d, expected
%d\n",
+ "A/D data dropout: received from channel %d,
expected %d\n",
chan, devpriv->act_chanlist[cur_chan]);
return -ENODATA;
}
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[PATCH v2 2/2] staging: comedi: vmk80xx: Fix possible double-free of ->usb_rx_buf
`vmk80xx_alloc_usb_buffers()` is called from `vmk80xx_auto_attach()` to
allocate RX and TX buffers for USB transfers. It allocates
`devpriv->usb_rx_buf` followed by `devpriv->usb_tx_buf`. If the
allocation of `devpriv->usb_tx_buf` fails, it frees
`devpriv->usb_rx_buf`, leaving the pointer set dangling, and returns an
error. Later, `vmk80xx_detach()` will be called from the core comedi
module code to clean up. `vmk80xx_detach()` also frees both
`devpriv->usb_rx_buf` and `devpriv->usb_tx_buf`, but
`devpriv->usb_rx_buf` may have already been freed, leading to a
double-free error. Fix it by removing the call to
`kfree(devpriv->usb_rx_buf)` from `vmk80xx_alloc_usb_buffers()`, relying
on `vmk80xx_detach()` to free the memory.
Signed-off-by: Ian Abbott
---
v2: Fix commit message because it incorrectly indicated that the call to
`kfree()` was being removed from `vmk80xx_auto_attach()`, not
`vmk80xx_alloc_usb_buffers().
---
drivers/staging/comedi/drivers/vmk80xx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/vmk80xx.c
b/drivers/staging/comedi/drivers/vmk80xx.c
index b035d662390b..65dc6c51037e 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -682,10 +682,8 @@ static int vmk80xx_alloc_usb_buffers(struct comedi_device
*dev)
size = usb_endpoint_maxp(devpriv->ep_tx);
devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
- if (!devpriv->usb_tx_buf) {
- kfree(devpriv->usb_rx_buf);
+ if (!devpriv->usb_tx_buf)
return -ENOMEM;
- }
return 0;
}
--
2.20.1
[PATCH 2/2] staging: comedi: ni_usb6501: Fix possible double-free of ->usb_rx_buf
`ni6501_alloc_usb_buffers()` is called from `ni6501_auto_attach()` to
allocate RX and TX buffers for USB transfers. It allocates
`devpriv->usb_rx_buf` followed by `devpriv->usb_tx_buf`. If the
allocation of `devpriv->usb_tx_buf` fails, it frees
`devpriv->usb_rx_buf`, leaving the pointer set dangling, and returns an
error. Later, `ni6501_detach()` will be called from the core comedi
module code to clean up. `ni6501_detach()` also frees both
`devpriv->usb_rx_buf` and `devpriv->usb_tx_buf`, but
`devpriv->usb_rx_buf` may have already beed freed, leading to a
double-free error. Fix it bu removing the call to
`kfree(devpriv->usb_rx_buf)` from `ni6501_alloc_usb_buffers()`, relying
on `ni6501_detach()` to free the memory.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/ni_usb6501.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c
b/drivers/staging/comedi/drivers/ni_usb6501.c
index ed5e42655821..1bb1cb651349 100644
--- a/drivers/staging/comedi/drivers/ni_usb6501.c
+++ b/drivers/staging/comedi/drivers/ni_usb6501.c
@@ -463,10 +463,8 @@ static int ni6501_alloc_usb_buffers(struct comedi_device
*dev)
size = usb_endpoint_maxp(devpriv->ep_tx);
devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
- if (!devpriv->usb_tx_buf) {
- kfree(devpriv->usb_rx_buf);
+ if (!devpriv->usb_tx_buf)
return -ENOMEM;
- }
return 0;
}
--
2.20.1
[PATCH 1/2] staging: comedi: ni_usb6501: Fix use of uninitialized mutex
If `ni6501_auto_attach()` returns an error, the core comedi module code will call `ni6501_detach()` to clean up. If `ni6501_auto_attach()` successfully allocated the comedi device private data, `ni6501_detach()` assumes that a `struct mutex mut` contained in the private data has been initialized and uses it. Unfortunately, there are a couple of places where `ni6501_auto_attach()` can return an error after allocating the device private data but before initializing the mutex, so this assumption is invalid. Fix it by initializing the mutex just after allocating the private data in `ni6501_auto_attach()` before any other errors can be retturned. Also move the call to `usb_set_intfdata()` just to keep the code a bit neater (either position for the call is fine). I believe this was the cause of the following syzbot crash report <https://syzkaller.appspot.com/bug?extid=cf4f2b6c24aff0a3edf6>: usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 usb 1-1: config 0 descriptor?? usb 1-1: string descriptor 0 read error: -71 comedi comedi0: Wrong number of endpoints ni6501 1-1:0.233: driver 'ni6501' failed to auto-configure device. INFO: trying to register non-static key. the code is fine but needs lockdep annotation. turning off the locking correctness validator. CPU: 0 PID: 585 Comm: kworker/0:3 Not tainted 5.1.0-rc4-319354-g9a33b36 #3 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xe8/0x16e lib/dump_stack.c:113 assign_lock_key kernel/locking/lockdep.c:786 [inline] register_lock_class+0x11b8/0x1250 kernel/locking/lockdep.c:1095 __lock_acquire+0xfb/0x37c0 kernel/locking/lockdep.c:3582 lock_acquire+0x10d/0x2f0 kernel/locking/lockdep.c:4211 __mutex_lock_common kernel/locking/mutex.c:925 [inline] __mutex_lock+0xfe/0x12b0 kernel/locking/mutex.c:1072 ni6501_detach+0x5b/0x110 drivers/staging/comedi/drivers/ni_usb6501.c:567 comedi_device_detach+0xed/0x800 drivers/staging/comedi/drivers.c:204 comedi_device_cleanup.part.0+0x68/0x140 drivers/staging/comedi/comedi_fops.c:156 comedi_device_cleanup drivers/staging/comedi/comedi_fops.c:187 [inline] comedi_free_board_dev.part.0+0x16/0x90 drivers/staging/comedi/comedi_fops.c:190 comedi_free_board_dev drivers/staging/comedi/comedi_fops.c:189 [inline] comedi_release_hardware_device+0x111/0x140 drivers/staging/comedi/comedi_fops.c:2880 comedi_auto_config.cold+0x124/0x1b0 drivers/staging/comedi/drivers.c:1068 usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361 really_probe+0x2da/0xb10 drivers/base/dd.c:509 driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 __device_attach+0x223/0x3a0 drivers/base/dd.c:844 bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 device_add+0xad2/0x16e0 drivers/base/core.c:2106 usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021 generic_probe+0xa2/0xda drivers/usb/core/generic.c:210 usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266 really_probe+0x2da/0xb10 drivers/base/dd.c:509 driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 __device_attach+0x223/0x3a0 drivers/base/dd.c:844 bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 device_add+0xad2/0x16e0 drivers/base/core.c:2106 usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534 hub_port_connect drivers/usb/core/hub.c:5089 [inline] hub_port_connect_change drivers/usb/core/hub.c:5204 [inline] port_event drivers/usb/core/hub.c:5350 [inline] hub_event+0x138e/0x3b00 drivers/usb/core/hub.c:5432 process_one_work+0x90f/0x1580 kernel/workqueue.c:2269 worker_thread+0x9b/0xe20 kernel/workqueue.c:2415 kthread+0x313/0x420 kernel/kthread.c:253 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352 Reported-by: syzbot+cf4f2b6c24aff0a3e...@syzkaller.appspotmail.com Signed-off-by: Ian Abbott --- drivers/staging/comedi/drivers/ni_usb6501.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c b/drivers/staging/comedi/drivers/ni_usb6501.c index 808ed92ed66f..ed5e42655821 100644 --- a/drivers/staging/comedi/drivers/ni_usb6501.c +++ b/drivers/staging/comedi/drivers/ni_usb6501.c @@ -518,6 +518,9 @@ static int ni6501_auto_attach(struct comedi_device *dev, if (!devpriv) return -ENOMEM; + mutex_init(>mut); + usb_set_intfdata(intf, devpriv); + ret = ni6501_find_endpoints(dev); if (ret) return ret; @@ -526,9 +529,6 @@ static int ni6501_auto_attach(struct comedi_device *dev, if (ret) return ret; - mutex_init(>mut); - usb_set_intfdata(intf, devpriv); - ret = comedi_alloc_subde
[PATCH 0/2] staging: comedi: ni_usb6501: Fix a couple of bugs
Fix a bug detected by syzbot and another that I found while investigating it. 1) staging: comedi: ni_usb6501: Fix use of uninitialized mutex 2) staging: comedi: ni_usb6501: Fix possible double-free of ->usb_rx_buf drivers/staging/comedi/drivers/ni_usb6501.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-)
[PATCH 2/2] staging: comedi: vmk80xx: Fix possible double-free of ->usb_rx_buf
`vmk80xx_alloc_usb_buffers()` is called from `vmk80xx_auto_attach()` to
allocate RX and TX buffers for USB transfers. It allocates
`devpriv->usb_rx_buf` followed by `devpriv->usb_tx_buf`. If the
allocation of `devpriv->usb_tx_buf` fails, it frees
`devpriv->usb_rx_buf`, leaving the pointer set dangling, and returns an
error. Later, `vmk80xx_detach()` will be called from the core comedi
module code to clean up. `vmk80xx_detach()` also frees both
`devpriv->usb_rx_buf` and `devpriv->usb_tx_buf`, but
`devpriv->usb_rx_buf` may have already been freed, leading to a
double-free error. Fix it by removing the call to
`kfree(devpriv->usb_rx_buf)` from `vmk80xx_auto_attach()`, relying on
`vmk80xx_detach()` to free the memory.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/vmk80xx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/vmk80xx.c
b/drivers/staging/comedi/drivers/vmk80xx.c
index b035d662390b..65dc6c51037e 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -682,10 +682,8 @@ static int vmk80xx_alloc_usb_buffers(struct comedi_device
*dev)
size = usb_endpoint_maxp(devpriv->ep_tx);
devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
- if (!devpriv->usb_tx_buf) {
- kfree(devpriv->usb_rx_buf);
+ if (!devpriv->usb_tx_buf)
return -ENOMEM;
- }
return 0;
}
--
2.20.1
[PATCH 1/2] staging: comedi: vmk80xx: Fix use of uninitialized semaphore
If `vmk80xx_auto_attach()` returns an error, the core comedi module code will call `vmk80xx_detach()` to clean up. If `vmk80xx_auto_attach()` successfully allocated the comedi device private data, `vmk80xx_detach()` assumes that a `struct semaphore limit_sem` contained in the private data has been initialized and uses it. Unfortunately, there are a couple of places where `vmk80xx_auto_attach()` can return an error after allocating the device private data but before initializing the semaphore, so this assumption is invalid. Fix it by initializing the semaphore just after allocating the private data in `vmk80xx_auto_attach()` before any other errors can be returned. I believe this was the cause of the following syzbot crash report <https://syzkaller.appspot.com/bug?extid=54c2f58f15fe6876b6ad>: usb 1-1: config 0 has no interface number 0 usb 1-1: New USB device found, idVendor=10cf, idProduct=8068, bcdDevice=e6.8d usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 usb 1-1: config 0 descriptor?? vmk80xx 1-1:0.117: driver 'vmk80xx' failed to auto-configure device. INFO: trying to register non-static key. the code is fine but needs lockdep annotation. turning off the locking correctness validator. CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xe8/0x16e lib/dump_stack.c:113 assign_lock_key kernel/locking/lockdep.c:786 [inline] register_lock_class+0x11b8/0x1250 kernel/locking/lockdep.c:1095 __lock_acquire+0xfb/0x37c0 kernel/locking/lockdep.c:3582 lock_acquire+0x10d/0x2f0 kernel/locking/lockdep.c:4211 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] _raw_spin_lock_irqsave+0x44/0x60 kernel/locking/spinlock.c:152 down+0x12/0x80 kernel/locking/semaphore.c:58 vmk80xx_detach+0x59/0x100 drivers/staging/comedi/drivers/vmk80xx.c:829 comedi_device_detach+0xed/0x800 drivers/staging/comedi/drivers.c:204 comedi_device_cleanup.part.0+0x68/0x140 drivers/staging/comedi/comedi_fops.c:156 comedi_device_cleanup drivers/staging/comedi/comedi_fops.c:187 [inline] comedi_free_board_dev.part.0+0x16/0x90 drivers/staging/comedi/comedi_fops.c:190 comedi_free_board_dev drivers/staging/comedi/comedi_fops.c:189 [inline] comedi_release_hardware_device+0x111/0x140 drivers/staging/comedi/comedi_fops.c:2880 comedi_auto_config.cold+0x124/0x1b0 drivers/staging/comedi/drivers.c:1068 usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361 really_probe+0x2da/0xb10 drivers/base/dd.c:509 driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 __device_attach+0x223/0x3a0 drivers/base/dd.c:844 bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 device_add+0xad2/0x16e0 drivers/base/core.c:2106 usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021 generic_probe+0xa2/0xda drivers/usb/core/generic.c:210 usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266 really_probe+0x2da/0xb10 drivers/base/dd.c:509 driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 __device_attach+0x223/0x3a0 drivers/base/dd.c:844 bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 device_add+0xad2/0x16e0 drivers/base/core.c:2106 usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534 hub_port_connect drivers/usb/core/hub.c:5089 [inline] hub_port_connect_change drivers/usb/core/hub.c:5204 [inline] port_event drivers/usb/core/hub.c:5350 [inline] hub_event+0x138e/0x3b00 drivers/usb/core/hub.c:5432 process_one_work+0x90f/0x1580 kernel/workqueue.c:2269 worker_thread+0x9b/0xe20 kernel/workqueue.c:2415 kthread+0x313/0x420 kernel/kthread.c:253 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352 Reported-by: syzbot+54c2f58f15fe6876b...@syzkaller.appspotmail.com Signed-off-by: Ian Abbott --- drivers/staging/comedi/drivers/vmk80xx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/vmk80xx.c b/drivers/staging/comedi/drivers/vmk80xx.c index 6234b649d887..b035d662390b 100644 --- a/drivers/staging/comedi/drivers/vmk80xx.c +++ b/drivers/staging/comedi/drivers/vmk80xx.c @@ -800,6 +800,8 @@ static int vmk80xx_auto_attach(struct comedi_device *dev, devpriv->model = board->model; + sema_init(>limit_sem, 8); + ret = vmk80xx_find_usb_endpoints(dev); if (ret) return ret; @@ -808,8 +810,6 @@ static int vmk80xx_auto_attach(struct comedi_device *dev, if (ret) return ret; - sema_init(>limit_sem, 8); - usb_set_intfdata(intf, devpriv); if (devpriv->model == VMK8055_MODEL) -- 2.20.1
[PATCH 0/2] staging: comedi: vmk80xx: Fix a couple of bugs
Fix a bug detected by syzbot and another bug that I noticed while investigating the syzbot reported bug. 1) staging: comedi: vmk80xx: Fix use of uninitialized semaphore 2) staging: comedi: vmk80xx: Fix possible double-free of ->usb_rx_buf drivers/staging/comedi/drivers/vmk80xx.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-)
Re: [PATCH -next] staging: comedi: dyna_pci10xx: remove set but not used variables 'chan' and range'
On 06/04/2019 04:07, YueHaibing wrote:
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/staging/comedi/drivers/dyna_pci10xx.c: In function
'dyna_pci10xx_insn_write_ao':
drivers/staging/comedi/drivers/dyna_pci10xx.c:109:21: warning:
variable 'range' set but not used [-Wunused-but-set-variable]
unsigned int chan, range;
drivers/staging/comedi/drivers/dyna_pci10xx.c:109:15: warning:
variable 'chan' set but not used [-Wunused-but-set-variable]
unsigned int chan, range;
They are never used since introduction in commit 16a7373a8e14 ("Staging:
comedi: add dyna_pci10xx driver")
Signed-off-by: YueHaibing
---
drivers/staging/comedi/drivers/dyna_pci10xx.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dyna_pci10xx.c
b/drivers/staging/comedi/drivers/dyna_pci10xx.c
index 9bdd5bf2eb99..d38bfc6113e8 100644
--- a/drivers/staging/comedi/drivers/dyna_pci10xx.c
+++ b/drivers/staging/comedi/drivers/dyna_pci10xx.c
@@ -106,10 +106,6 @@ static int dyna_pci10xx_insn_write_ao(struct comedi_device
*dev,
{
struct dyna_pci10xx_private *devpriv = dev->private;
int n;
- unsigned int chan, range;
-
- chan = CR_CHAN(insn->chanspec);
- range = range_codes_pci1050_ai[CR_RANGE((insn->chanspec))];
mutex_lock(>mutex);
for (n = 0; n < insn->n; n++) {
That seems fine, thanks. The lines being removed appear to have been
copy and pasted from dyna_pci10xx_insn_read_ai(). The AO subdevice
supports a single range, so no 'range' code is needed. I believe the
card has only one analog output channel[*], so the 'chan' is irrelevant.
This means the 'n_chans' value in the AO subdevice configuration is
incorrect. I'll send a follow-up patch to correct that after Greg KH
takes this one.
[*] Very little information seems to be available about this card
online. It is no longer listed on the manufacturer's web-site and I
couldn't find any archives on archive.org. I found part of a scientific
paper
<http://shodhganga.inflibnet.ac.in/bitstream/10603/150646/15/15_chapter%205.pdf>
which describes the card as having a single channel D/A converter.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH V2] staging: comedi: dt2811: Fix spelling mistake
On 06/04/2019 09:23, Hariprasad Kelam wrote: changes interupts --> interrupts to fix warning reported by checkpatch tool Signed-off-by: Hariprasad Kelam --- Changes in v2: - Make the Subject more clear by including changed file path. --- drivers/staging/comedi/drivers/dt2811.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/dt2811.c b/drivers/staging/comedi/drivers/dt2811.c index 05207a5..8a1f9ef 100644 --- a/drivers/staging/comedi/drivers/dt2811.c +++ b/drivers/staging/comedi/drivers/dt2811.c @@ -52,7 +52,7 @@ #define DT2811_ADCSR_ADBUSY BIT(5) /* r 1=A/D busy */ #define DT2811_ADCSR_CLRERROR BIT(4) #define DT2811_ADCSR_DMAENB BIT(3) /* r/w1=dma ena */ -#define DT2811_ADCSR_INTENBBIT(2) /* r/w1=interupts ena */ +#define DT2811_ADCSR_INTENBBIT(2) /* r/w1=interrupts ena */ #define DT2811_ADCSR_ADMODE(x)(((x) & 0x3) << 0) #define DT2811_ADGCR_REG 0x01 /* r/w A/D Gain/Channel */ Looks good. Thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: Fix spelling mistake
On 04/04/2019 02:24, Hariprasad Kelam wrote: changes interupts --> interrupts to fix warning reported by checkpatch tool Signed-off-by: Hariprasad Kelam --- drivers/staging/comedi/drivers/dt2811.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/dt2811.c b/drivers/staging/comedi/drivers/dt2811.c index 05207a5..8a1f9ef 100644 --- a/drivers/staging/comedi/drivers/dt2811.c +++ b/drivers/staging/comedi/drivers/dt2811.c @@ -52,7 +52,7 @@ #define DT2811_ADCSR_ADBUSY BIT(5) /* r 1=A/D busy */ #define DT2811_ADCSR_CLRERROR BIT(4) #define DT2811_ADCSR_DMAENB BIT(3) /* r/w1=dma ena */ -#define DT2811_ADCSR_INTENBBIT(2) /* r/w1=interupts ena */ +#define DT2811_ADCSR_INTENBBIT(2) /* r/w1=interrupts ena */ #define DT2811_ADCSR_ADMODE(x)(((x) & 0x3) << 0) #define DT2811_ADGCR_REG 0x01 /* r/w A/D Gain/Channel */ Hi Hariprasad, could you mention dt2811 in the patch subject line like this: staging: comedi: dt2811: Fix spelling mistake It helps to narrow down the scope of the patch. Apart from that, the patch looks fine. Thanks. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[RESEND PATCH v1] fuse: Add ioctl flag for compat ioctl with 64-bit time_t
Currently, a CUSE server running on a 64-bit kernel can tell when an
ioctl request comes from a process running a 32-bit ABI, but cannot tell
whether the requesting process is using legacy IA32 emulation or x32
ABI, for example. In particular, the server does not know the size of
the client process's `time_t` type.
For 64-bit kernels, the `FUSE_IOCTL_COMPAT` and `FUSE_IOCTL_32BIT` flags
are currently set in the ioctl input request (`struct fuse_ioctl_in`
member `flags`) for a 32-bit requesting process. This patch defines a
new flag `FUSE_IOCTL_COMPAT_64TIME` and sets it if the 32-bit requesting
process (running on a 64-bit kernel) uses a 64-bit `time_t` type.
Signed-off-by: Ian Abbott
---
This is a resend of a patch I sent on March 1. No changes.
https://www.spinics.net/lists/linux-fsdevel/msg140531.html
fs/fuse/file.c| 5 -
include/uapi/linux/fuse.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 06096b60f1df..9777e7a19889 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2576,8 +2576,11 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd,
unsigned long arg,
#if BITS_PER_LONG == 32
inarg.flags |= FUSE_IOCTL_32BIT;
#else
- if (flags & FUSE_IOCTL_COMPAT)
+ if (flags & FUSE_IOCTL_COMPAT) {
inarg.flags |= FUSE_IOCTL_32BIT;
+ if (COMPAT_USE_64BIT_TIME)
+ inarg.flags |= FUSE_IOCTL_COMPAT_64TIME;
+ }
#endif
/* assume all the iovs returned by client always fits in a page */
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2ac598614a8f..1f4a71486601 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -335,6 +335,7 @@ struct fuse_file_lock {
* FUSE_IOCTL_RETRY: retry with new iovecs
* FUSE_IOCTL_32BIT: 32bit ioctl
* FUSE_IOCTL_DIR: is a directory
+ * FUSE_IOCTL_COMPAT_64TIME: 32bit compat ioctl with 64bit time_t
*
* FUSE_IOCTL_MAX_IOV: maximum of in_iovecs + out_iovecs
*/
@@ -343,6 +344,7 @@ struct fuse_file_lock {
#define FUSE_IOCTL_RETRY (1 << 2)
#define FUSE_IOCTL_32BIT (1 << 3)
#define FUSE_IOCTL_DIR (1 << 4)
+#define FUSE_IOCTL_COMPAT_64TIME (1 << 5)
#define FUSE_IOCTL_MAX_IOV 256
--
2.20.1
Re: [PATCH] Staging: comedi: ni_mio_common.c: Added blank line after declarations
On 21/03/2019 19:18, Arash Fotouhi wrote:
Added blank line after declarations.
Signed-off-by: Arash Fotouhi
---
drivers/staging/comedi/drivers/ni_mio_common.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 5edf59a..c6aff8f 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2110,6 +2110,7 @@ static int ni_ai_cmdtest(struct comedi_device *dev,
struct comedi_subdevice *s,
if (cmd->scan_begin_src == TRIG_TIMER) {
unsigned int tmp = cmd->scan_begin_arg;
+
cmd->scan_begin_arg =
ni_timer_to_ns(dev, ni_ns_to_timer(dev,
cmd->scan_begin_arg,
@@ -2120,6 +2121,7 @@ static int ni_ai_cmdtest(struct comedi_device *dev,
struct comedi_subdevice *s,
if (cmd->convert_src == TRIG_TIMER) {
if (!devpriv->is_611x && !devpriv->is_6143) {
unsigned int tmp = cmd->convert_arg;
+
cmd->convert_arg =
ni_timer_to_ns(dev, ni_ns_to_timer(dev,
cmd->convert_arg,
Looks good, thanks.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On 04/03/2019 14:33, Ian Abbott wrote:
`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO
subdevice (subdevice 2) of supported National Instruments M-series
cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST`
ioctls for this subdevice. There are two causes for a possible
divide-by-zero error when validating that the `stop_arg` member of the
passed-in command is not too large.
The first cause for the divide-by-zero is that calls to
`comedi_bytes_per_scan()` are only valid once the command has been
copied to `s->async->cmd`, but that copy is only done for the
`COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use
whatever was left there by the previous `COMEDI_CMD` ioctl, if any.
(This is very likely, as it is usual for the application to use
`COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous,
valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()`
will return 0, so the subsequent division in `ni_cdio_cmdtest()` of
`s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a
divide-by-zero error. To fix this error, call a new function
`comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing
`comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for
its calculations. (Also refactor `comedi_bytes_per_scan()` to call the
new function.)
Once the first cause for the divide-by-zero has been fixed, the second
cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if
the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0.
Fix it by only performing the division (and validating that `stop_arg`
is no more than the maximum value) if `comedi_bytes_per_scan_cmd()`
returns a non-zero value.
The problem was reported on the COMEDI mailing list here:
https://groups.google.com/forum/#!topic/comedi_list/4t9WlHzMhKM
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio
output")
Greg,
If it's not too late, it would be nice if the following "Reported-by:"
and "Tested-by:" lines could be added (or I can resend with these lines
included if necessary). It's no big deal if this is too late. I'll
live with it. Thanks.
Reported-by: Ivan Vasilyev
Tested-by: Ivan Vasilyev
Cc: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/comedidev.h| 2 ++
drivers/staging/comedi/drivers.c | 33 ---
.../staging/comedi/drivers/ni_mio_common.c| 10 --
3 files changed, 38 insertions(+), 7 deletions(-)
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On 05/03/2019 11:39, Dan Carpenter wrote: On Tue, Mar 05, 2019 at 11:32:18AM +, Ian Abbott wrote: On 05/03/2019 11:10, Dan Carpenter wrote: On Mon, Mar 04, 2019 at 02:33:54PM +, Ian Abbott wrote: The problem was reported on the COMEDI mailing list here: https://groups.google.com/forum/#!topic/comedi_list/4t9WlHzMhKM Can you give Ivan a Reported-by tag? It's a public mailing list so that shouldn't be a problem. I can do, but I don't know his full name. Will that be a problem? Nah, it's not a problem. People should fix their email headers to reflect what they want to be called. Or you could ask but I don't think I have ever asked for someone's name I've always just gone with their email header name. In this case, Ivan just signed off with that name and it didn't appear in the email headers. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
On 05/03/2019 11:10, Dan Carpenter wrote:
On Mon, Mar 04, 2019 at 02:33:54PM +, Ian Abbott wrote:
The problem was reported on the COMEDI mailing list here:
https://groups.google.com/forum/#!topic/comedi_list/4t9WlHzMhKM
Can you give Ivan a Reported-by tag? It's a public mailing list so
that shouldn't be a problem.
I can do, but I don't know his full name. Will that be a problem?
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration to dio
output")
Cc: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
regards,
dan carpenter
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
[PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO
subdevice (subdevice 2) of supported National Instruments M-series
cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST`
ioctls for this subdevice. There are two causes for a possible
divide-by-zero error when validating that the `stop_arg` member of the
passed-in command is not too large.
The first cause for the divide-by-zero is that calls to
`comedi_bytes_per_scan()` are only valid once the command has been
copied to `s->async->cmd`, but that copy is only done for the
`COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use
whatever was left there by the previous `COMEDI_CMD` ioctl, if any.
(This is very likely, as it is usual for the application to use
`COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous,
valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()`
will return 0, so the subsequent division in `ni_cdio_cmdtest()` of
`s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a
divide-by-zero error. To fix this error, call a new function
`comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing
`comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for
its calculations. (Also refactor `comedi_bytes_per_scan()` to call the
new function.)
Once the first cause for the divide-by-zero has been fixed, the second
cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if
the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0.
Fix it by only performing the division (and validating that `stop_arg`
is no more than the maximum value) if `comedi_bytes_per_scan_cmd()`
returns a non-zero value.
The problem was reported on the COMEDI mailing list here:
https://groups.google.com/forum/#!topic/comedi_list/4t9WlHzMhKM
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration
to dio output")
Cc: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/comedidev.h| 2 ++
drivers/staging/comedi/drivers.c | 33 ---
.../staging/comedi/drivers/ni_mio_common.c| 10 --
3 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/comedi/comedidev.h
b/drivers/staging/comedi/comedidev.h
index a7d569cfca5d..0dff1ac057cd 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -1001,6 +1001,8 @@ int comedi_dio_insn_config(struct comedi_device *dev,
unsigned int mask);
unsigned int comedi_dio_update_state(struct comedi_subdevice *s,
unsigned int *data);
+unsigned int comedi_bytes_per_scan_cmd(struct comedi_subdevice *s,
+ struct comedi_cmd *cmd);
unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s);
unsigned int comedi_nscans_left(struct comedi_subdevice *s,
unsigned int nscans);
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index eefa62f42c0f..5a32b8fc000e 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -394,11 +394,13 @@ unsigned int comedi_dio_update_state(struct
comedi_subdevice *s,
EXPORT_SYMBOL_GPL(comedi_dio_update_state);
/**
- * comedi_bytes_per_scan() - Get length of asynchronous command "scan" in bytes
+ * comedi_bytes_per_scan_cmd() - Get length of asynchronous command "scan" in
+ * bytes
* @s: COMEDI subdevice.
+ * @cmd: COMEDI command.
*
* Determines the overall scan length according to the subdevice type and the
- * number of channels in the scan.
+ * number of channels in the scan for the specified command.
*
* For digital input, output or input/output subdevices, samples for
* multiple channels are assumed to be packed into one or more unsigned
@@ -408,9 +410,9 @@ EXPORT_SYMBOL_GPL(comedi_dio_update_state);
*
* Returns the overall scan length in bytes.
*/
-unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s)
+unsigned int comedi_bytes_per_scan_cmd(struct comedi_subdevice *s,
+ struct comedi_cmd *cmd)
{
- struct comedi_cmd *cmd = >async->cmd;
unsigned int num_samples;
unsigned int bits_per_sample;
@@ -427,6 +429,29 @@ unsigned int comedi_bytes_per_scan(struct comedi_subdevice
*s)
}
return comedi_samples_to_bytes(s, num_samples);
}
+EXPORT_SYMBOL_GPL(comedi_bytes_per_scan_cmd);
+
+/**
+ * comedi_bytes_per_scan() - Get length of asynchronous command "scan" in bytes
+ * @s: COMEDI subdevice.
+ *
+ * Determines the overall scan length according to the subdevice type and the
+ * number of channels in the scan for the current command.
+ *
+ * For digital input, output or input/output subdevices, samples for
+ * multiple channels are assumed to be packed into one or more unsigned
+ * short or un
[PATCH] fuse: Add ioctl flag for compat ioctl with 64-bit time_t
Currently, a CUSE server running on a 64-bit kernel can tell when an
ioctl request comes from a process running a 32-bit ABI, but cannot tell
whether the requesting process is using legacy IA32 emulation or x32
ABI, for example. In particular, the server does not know the size of
the client process's `time_t` type.
For 64-bit kernels, the `FUSE_IOCTL_COMPAT` and `FUSE_IOCTL_32BIT` flags
are currently set in the ioctl input request (`struct fuse_ioctl_in`
member `flags`) for a 32-bit requesting process. This patch defines a
new flag `FUSE_IOCTL_COMPAT_64TIME` and sets it if the 32-bit requesting
process (running on a 64-bit kernel) uses a 64-bit `time_t` type.
Signed-off-by: Ian Abbott
---
fs/fuse/file.c| 5 -
include/uapi/linux/fuse.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 06096b60f1df..9777e7a19889 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2576,8 +2576,11 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd,
unsigned long arg,
#if BITS_PER_LONG == 32
inarg.flags |= FUSE_IOCTL_32BIT;
#else
- if (flags & FUSE_IOCTL_COMPAT)
+ if (flags & FUSE_IOCTL_COMPAT) {
inarg.flags |= FUSE_IOCTL_32BIT;
+ if (COMPAT_USE_64BIT_TIME)
+ inarg.flags |= FUSE_IOCTL_COMPAT_64TIME;
+ }
#endif
/* assume all the iovs returned by client always fits in a page */
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2ac598614a8f..1f4a71486601 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -335,6 +335,7 @@ struct fuse_file_lock {
* FUSE_IOCTL_RETRY: retry with new iovecs
* FUSE_IOCTL_32BIT: 32bit ioctl
* FUSE_IOCTL_DIR: is a directory
+ * FUSE_IOCTL_COMPAT_64TIME: 32bit compat ioctl with 64bit time_t
*
* FUSE_IOCTL_MAX_IOV: maximum of in_iovecs + out_iovecs
*/
@@ -343,6 +344,7 @@ struct fuse_file_lock {
#define FUSE_IOCTL_RETRY (1 << 2)
#define FUSE_IOCTL_32BIT (1 << 3)
#define FUSE_IOCTL_DIR (1 << 4)
+#define FUSE_IOCTL_COMPAT_64TIME (1 << 5)
#define FUSE_IOCTL_MAX_IOV 256
--
2.20.1
Re: [PATCH] staging: comedi: ni_660x: fix missing break in switch statement
On 15/02/2019 15:48, Sasha Levin wrote:
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 58dd7c0a2a6e Staging: comedi: add ni_660x driver.
The bot has tested the following trees: v4.20.8, v4.19.21, v4.14.99, v4.9.156,
v4.4.174, v3.18.134.
v4.20.8: Build OK!
v4.19.21: Build OK!
v4.14.99: Build OK!
v4.9.156: Build OK!
v4.4.174: Failed to apply! Possible dependencies:
01ead0ded315 ("staging: comedi: ni_660x: cleanup the NI660X_IO_CFG register
helpers")
22acd860137a ("staging: comedi: ni_660x: change IOConfigReg() into a
macro")
41014593caeb ("staging: comedi: ni_660x: cleanup the NI660X_GLOBAL_INT_{STATUS,
CFG}")
502552e161ae ("staging: comedi: ni_660x: remove enum
clock_config_register_bits")
518d38423b48 ("staging: comedi: ni_660x: tidy up
ni_660x_select_pfi_output()")
9678b73e273a ("staging: comedi: ni_660x: tidy up ni_660x_write_register()")
aa94f225 ("staging: comedi: ni_660x: tidy up
ni_660x_set_pfi_routing()")
ad98c18cb9de ("staging: comedi: ni_660x: tidy up ni_660x_read_register()")
cded944fa90c ("staging: comedi: ni_660x: Prefer kernel type 'u64' over
'uint64_t'")
fecf4cce0021 ("staging: comedi: ni_660x: cleanup the NI660X_DMA_CFG register
helpers")
v3.18.134: Failed to apply! Possible dependencies:
01ead0ded315 ("staging: comedi: ni_660x: cleanup the NI660X_IO_CFG register
helpers")
22acd860137a ("staging: comedi: ni_660x: change IOConfigReg() into a
macro")
41014593caeb ("staging: comedi: ni_660x: cleanup the NI660X_GLOBAL_INT_{STATUS,
CFG}")
502552e161ae ("staging: comedi: ni_660x: remove enum
clock_config_register_bits")
518d38423b48 ("staging: comedi: ni_660x: tidy up
ni_660x_select_pfi_output()")
9678b73e273a ("staging: comedi: ni_660x: tidy up ni_660x_write_register()")
aa94f225 ("staging: comedi: ni_660x: tidy up
ni_660x_set_pfi_routing()")
ad98c18cb9de ("staging: comedi: ni_660x: tidy up ni_660x_read_register()")
cded944fa90c ("staging: comedi: ni_660x: Prefer kernel type 'u64' over
'uint64_t'")
fecf4cce0021 ("staging: comedi: ni_660x: cleanup the NI660X_DMA_CFG register
helpers")
How should we proceed with this patch?
Hi Sasha, the bug was introduced in v4.7 and hasn't been backported to
any earlier stable kernels, so no need to do anything for v4.4.x or v3.18.x.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH v2] staging: comedi: ni_660x: fix missing break in switch statement
On 12/02/2019 18:44, Gustavo A. R. Silva wrote:
Add missing break statement in order to prevent the code from falling
through to the default case and return -EINVAL every time.
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
Fixes: aa94f225 ("staging: comedi: ni_660x: tidy up
ni_660x_set_pfi_routing()")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Fix Fixes tag.
drivers/staging/comedi/drivers/ni_660x.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/comedi/drivers/ni_660x.c
b/drivers/staging/comedi/drivers/ni_660x.c
index e70a461e723f..405573e927cf 100644
--- a/drivers/staging/comedi/drivers/ni_660x.c
+++ b/drivers/staging/comedi/drivers/ni_660x.c
@@ -656,6 +656,7 @@ static int ni_660x_set_pfi_routing(struct comedi_device
*dev,
case NI_660X_PFI_OUTPUT_DIO:
if (chan > 31)
return -EINVAL;
+ break;
default:
return -EINVAL;
}
Thanks for the bug fix!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: Removed not necessary braces for single block
On 15/01/2019 15:36, Jitendra Khasdev wrote:
This patch is used to remove not necessary braces for single if block.
Signed-off-by: Jitendra Khasdev
---
drivers/staging/comedi/comedi_fops.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index 5d2fcbfe02af..38980fad8be4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1605,9 +1605,9 @@ static int do_insn_ioctl(struct comedi_device *dev,
unsigned int n_data = MIN_SAMPLES;
int ret = 0;
- if (copy_from_user(, arg, sizeof(insn))) {
+ if (copy_from_user(, arg, sizeof(insn)))
return -EFAULT;
- }
+
n_data = max(n_data, insn.n);
The patch looks fine. Thanks for the fix!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: fix spelling mistake "desination" -> "destination"
On 27/11/2018 14:23, Colin King wrote: From: Colin Ian King There is a spelling mistake in message text in the call to unittest, fix this. Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/tests/ni_routes_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c index a1eda035f270..c6dc18f346e8 100644 --- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c +++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c @@ -372,7 +372,7 @@ void test_ni_lookup_route_register(void) unittest(ni_lookup_route_register(O(8), O(9), T) == 8, "validate last destination\n"); unittest(ni_lookup_route_register(O(10), O(9), T) == -EINVAL, -"lookup invalid desination\n"); +"lookup invalid destination\n"); unittest(ni_lookup_route_register(rgout0_src0, TRIGGER_LINE(0), T) == -EINVAL, Thanks for spotting that! Looks good. Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: fix spelling mistake "desination" -> "destination"
On 27/11/2018 14:23, Colin King wrote: From: Colin Ian King There is a spelling mistake in message text in the call to unittest, fix this. Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/tests/ni_routes_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c index a1eda035f270..c6dc18f346e8 100644 --- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c +++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c @@ -372,7 +372,7 @@ void test_ni_lookup_route_register(void) unittest(ni_lookup_route_register(O(8), O(9), T) == 8, "validate last destination\n"); unittest(ni_lookup_route_register(O(10), O(9), T) == -EINVAL, -"lookup invalid desination\n"); +"lookup invalid destination\n"); unittest(ni_lookup_route_register(rgout0_src0, TRIGGER_LINE(0), T) == -EINVAL, Thanks for spotting that! Looks good. Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: tio: fix multiple missing break in switch bugs
On 11/10/2018 20:05, Gustavo A. R. Silva wrote:
Currently, there are multiple missing break statements in two switch code
blocks. This makes the execution path to fall all the way down through
to the default cases, which makes the function ni_tio_set_gate_src() to
always return -EINVAL.
Fix this by adding the missing break statements.
Also, notice that due to the absence of the break statements,
the following pieces of code are unreachable:
1078if (ret)
1079return ret;
1080/* 3. reenable & set mode to starts things back up */
1081ni_tio_set_gate_mode(counter, src);
1098if (ret)
1099return ret;
1100/* 3. reenable & set mode to starts things back up */
1101ni_tio_set_gate2_mode(counter, src);
So, by adding the missing breaks, this patch also fixes the problem
above.
Addresses-Coverity-ID: 1474165 ("Missing break in switch")
Addresses-Coverity-ID: 1474162 ("Structurally dead code")
Fixes: 347e244884c3 ("staging: comedi: tio: implement global tio/ctr routing")
Signed-off-by: Gustavo A. R. Silva
---
drivers/staging/comedi/drivers/ni_tio.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/staging/comedi/drivers/ni_tio.c
b/drivers/staging/comedi/drivers/ni_tio.c
index 838614e..0eb388c 100644
--- a/drivers/staging/comedi/drivers/ni_tio.c
+++ b/drivers/staging/comedi/drivers/ni_tio.c
@@ -1070,8 +1070,10 @@ int ni_tio_set_gate_src(struct ni_gpct *counter,
case ni_gpct_variant_e_series:
case ni_gpct_variant_m_series:
ret = ni_m_set_gate(counter, chan);
+ break;
case ni_gpct_variant_660x:
ret = ni_660x_set_gate(counter, chan);
+ break;
default:
return -EINVAL;
}
@@ -1090,8 +1092,10 @@ int ni_tio_set_gate_src(struct ni_gpct *counter,
switch (counter_dev->variant) {
case ni_gpct_variant_m_series:
ret = ni_m_set_gate2(counter, chan);
+ break;
case ni_gpct_variant_660x:
ret = ni_660x_set_gate2(counter, chan);
+ break;
default:
return -EINVAL;
}
Thanks for catching that bug!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: tio: fix multiple missing break in switch bugs
On 11/10/2018 20:05, Gustavo A. R. Silva wrote:
Currently, there are multiple missing break statements in two switch code
blocks. This makes the execution path to fall all the way down through
to the default cases, which makes the function ni_tio_set_gate_src() to
always return -EINVAL.
Fix this by adding the missing break statements.
Also, notice that due to the absence of the break statements,
the following pieces of code are unreachable:
1078if (ret)
1079return ret;
1080/* 3. reenable & set mode to starts things back up */
1081ni_tio_set_gate_mode(counter, src);
1098if (ret)
1099return ret;
1100/* 3. reenable & set mode to starts things back up */
1101ni_tio_set_gate2_mode(counter, src);
So, by adding the missing breaks, this patch also fixes the problem
above.
Addresses-Coverity-ID: 1474165 ("Missing break in switch")
Addresses-Coverity-ID: 1474162 ("Structurally dead code")
Fixes: 347e244884c3 ("staging: comedi: tio: implement global tio/ctr routing")
Signed-off-by: Gustavo A. R. Silva
---
drivers/staging/comedi/drivers/ni_tio.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/staging/comedi/drivers/ni_tio.c
b/drivers/staging/comedi/drivers/ni_tio.c
index 838614e..0eb388c 100644
--- a/drivers/staging/comedi/drivers/ni_tio.c
+++ b/drivers/staging/comedi/drivers/ni_tio.c
@@ -1070,8 +1070,10 @@ int ni_tio_set_gate_src(struct ni_gpct *counter,
case ni_gpct_variant_e_series:
case ni_gpct_variant_m_series:
ret = ni_m_set_gate(counter, chan);
+ break;
case ni_gpct_variant_660x:
ret = ni_660x_set_gate(counter, chan);
+ break;
default:
return -EINVAL;
}
@@ -1090,8 +1092,10 @@ int ni_tio_set_gate_src(struct ni_gpct *counter,
switch (counter_dev->variant) {
case ni_gpct_variant_m_series:
ret = ni_m_set_gate2(counter, chan);
+ break;
case ni_gpct_variant_660x:
ret = ni_660x_set_gate2(counter, chan);
+ break;
default:
return -EINVAL;
}
Thanks for catching that bug!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 1/4] staging: comedi: Correct multiline dereference as per TODO
On 03/09/18 15:47, Ian Abbott wrote: On 30/08/18 18:32, Ray Clinton wrote: Using checkpatch.pl I was able to find a multiline dereference which goes again the coding style for the kernel. I'm still working on my email client so the indentation looks bad here (in gmail) but the arguments for comedi_check_trigger_arg_min should go just under the opening ( The patch description should describe what the patch does, not how bad the indentation loos in your email client. "loos" ==> "looks" Also, you sent 4 patches with the same subject line (other than the PATCH 1/4 part). They should be different. You can include the driver name in the patch subject line to distinguish them, e.g.: staging: comedi: dt3000: Correct multiline dereference -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 1/4] staging: comedi: Correct multiline dereference as per TODO
On 03/09/18 15:47, Ian Abbott wrote: On 30/08/18 18:32, Ray Clinton wrote: Using checkpatch.pl I was able to find a multiline dereference which goes again the coding style for the kernel. I'm still working on my email client so the indentation looks bad here (in gmail) but the arguments for comedi_check_trigger_arg_min should go just under the opening ( The patch description should describe what the patch does, not how bad the indentation loos in your email client. "loos" ==> "looks" Also, you sent 4 patches with the same subject line (other than the PATCH 1/4 part). They should be different. You can include the driver name in the patch subject line to distinguish them, e.g.: staging: comedi: dt3000: Correct multiline dereference -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 1/4] staging: comedi: Correct multiline dereference as per TODO
On 30/08/18 18:32, Ray Clinton wrote:
Using checkpatch.pl I was able to find a multiline dereference which goes
again the coding style for the kernel. I'm still working on my email client so
the indentation looks bad here (in gmail) but the arguments for
comedi_check_trigger_arg_min should go just under the opening (
The patch description should describe what the patch does, not how bad
the indentation loos in your email client.
Signed-off-by: Ray Clinton
---
drivers/staging/comedi/drivers/dt3000.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt3000.c
b/drivers/staging/comedi/drivers/dt3000.c
index 2edf3ee..6bc5c86 100644
--- a/drivers/staging/comedi/drivers/dt3000.c
+++ b/drivers/staging/comedi/drivers/dt3000.c
@@ -439,9 +439,9 @@ static int dt3k_ai_cmdtest(struct comedi_device *dev,
if (cmd->scan_begin_src == TRIG_TIMER) {
arg = cmd->convert_arg * cmd->scan_end_arg;
- err |= comedi_check_trigger_arg_min(>
- scan_begin_arg,
- arg);
+ err |= comedi_check_trigger_arg_min(
+ >scan_begin_arg,
+ arg);
}
}
--
2.7.4
I can't get the patch to apply. Something is converting tabs to spaces,
and I assume it is your email client doing that. I recommend using the
git send-email command to send the patches, after configuring your git
to send emails via Gmail. You can google how to do that, but here is a
starting point: https://gist.github.com/jasonkarns/4354421
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH 1/4] staging: comedi: Correct multiline dereference as per TODO
On 30/08/18 18:32, Ray Clinton wrote:
Using checkpatch.pl I was able to find a multiline dereference which goes
again the coding style for the kernel. I'm still working on my email client so
the indentation looks bad here (in gmail) but the arguments for
comedi_check_trigger_arg_min should go just under the opening (
The patch description should describe what the patch does, not how bad
the indentation loos in your email client.
Signed-off-by: Ray Clinton
---
drivers/staging/comedi/drivers/dt3000.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt3000.c
b/drivers/staging/comedi/drivers/dt3000.c
index 2edf3ee..6bc5c86 100644
--- a/drivers/staging/comedi/drivers/dt3000.c
+++ b/drivers/staging/comedi/drivers/dt3000.c
@@ -439,9 +439,9 @@ static int dt3k_ai_cmdtest(struct comedi_device *dev,
if (cmd->scan_begin_src == TRIG_TIMER) {
arg = cmd->convert_arg * cmd->scan_end_arg;
- err |= comedi_check_trigger_arg_min(>
- scan_begin_arg,
- arg);
+ err |= comedi_check_trigger_arg_min(
+ >scan_begin_arg,
+ arg);
}
}
--
2.7.4
I can't get the patch to apply. Something is converting tabs to spaces,
and I assume it is your email client doing that. I recommend using the
git send-email command to send the patches, after configuring your git
to send emails via Gmail. You can google how to do that, but here is a
starting point: https://gist.github.com/jasonkarns/4354421
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: comedi_fops: Shift assignment operator '=' to previous line
On 14/07/18 16:44, Nishad Kamdar wrote: Shift '=' assignment operator to the end of previous line to conform to preferred kernel style line wrapping. Issue reported by checkpatch CHECK. Signed-off-by: Nishad Kamdar --- drivers/staging/comedi/comedi_fops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 1f3b1106f478..e18b61cdbdeb 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -79,8 +79,8 @@ MODULE_PARM_DESC(comedi_default_buf_size_kb, "default asynchronous buffer size in KiB (default " __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB) ")"); -unsigned int comedi_default_buf_maxsize_kb - = CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB; +unsigned int comedi_default_buf_maxsize_kb = + CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB; module_param(comedi_default_buf_maxsize_kb, uint, 0644); MODULE_PARM_DESC(comedi_default_buf_maxsize_kb, "default maximum size of asynchronous buffer in KiB (default " That looks fine, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: comedi_fops: Shift assignment operator '=' to previous line
On 14/07/18 16:44, Nishad Kamdar wrote: Shift '=' assignment operator to the end of previous line to conform to preferred kernel style line wrapping. Issue reported by checkpatch CHECK. Signed-off-by: Nishad Kamdar --- drivers/staging/comedi/comedi_fops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 1f3b1106f478..e18b61cdbdeb 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -79,8 +79,8 @@ MODULE_PARM_DESC(comedi_default_buf_size_kb, "default asynchronous buffer size in KiB (default " __MODULE_STRING(CONFIG_COMEDI_DEFAULT_BUF_SIZE_KB) ")"); -unsigned int comedi_default_buf_maxsize_kb - = CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB; +unsigned int comedi_default_buf_maxsize_kb = + CONFIG_COMEDI_DEFAULT_BUF_MAXSIZE_KB; module_param(comedi_default_buf_maxsize_kb, uint, 0644); MODULE_PARM_DESC(comedi_default_buf_maxsize_kb, "default maximum size of asynchronous buffer in KiB (default " That looks fine, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: remove redundant variable segpos
On 11/07/18 11:32, Colin King wrote: From: Colin Ian King Variable segpos is being assigned but is never used hence it is redundant and can be removed. Cleans up clang warning: warning: variable 'segpos' set but not used [-Wunused-but-set-variable] Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/pcl816.c | 4 ++-- drivers/staging/comedi/drivers/pcl818.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) Looks good, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: remove redundant variable segpos
On 11/07/18 11:32, Colin King wrote: From: Colin Ian King Variable segpos is being assigned but is never used hence it is redundant and can be removed. Cleans up clang warning: warning: variable 'segpos' set but not used [-Wunused-but-set-variable] Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/pcl816.c | 4 ++-- drivers/staging/comedi/drivers/pcl818.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) Looks good, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: can we drop the comedi serial2002 drivers from staging?
On 16/06/18 10:37, Greg Kroah-Hartman wrote: On Sat, Jun 16, 2018 at 11:06:45AM +0200, Christoph Hellwig wrote: As far as I can tell there has been no targeted work on the driver since merging it at all, and since 2014 even the comedi-wide cleanups stopped, laving just drive by tree wide changes since. At the same time the driver badly abuses the tty layer and is one of only two major abusers of the poll code for in-kernel waits. Yeah, it's an ugly driver. It should be rewritten to use the new serdev api instead of trying to open a tty device node from within the kernel. I was thinking about that, but it looks tricky to use for a "normal" PC-based distro due to typical lack of device-tree support. Another possibility is to support comedi drivers in userspace via CUSE, which would require a comedi-cuse interface library that user-space comedi drivers could link to similar to how kernel-mode comedi drivers link to the comedi core. I don't object to dropping it. Ian and Hartley, do you know of any users of this driver anymore? The only place I know it was in use was in Anders Blomdell's lab at Lund University. I've added him to the Cc: list. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: can we drop the comedi serial2002 drivers from staging?
On 16/06/18 10:37, Greg Kroah-Hartman wrote: On Sat, Jun 16, 2018 at 11:06:45AM +0200, Christoph Hellwig wrote: As far as I can tell there has been no targeted work on the driver since merging it at all, and since 2014 even the comedi-wide cleanups stopped, laving just drive by tree wide changes since. At the same time the driver badly abuses the tty layer and is one of only two major abusers of the poll code for in-kernel waits. Yeah, it's an ugly driver. It should be rewritten to use the new serdev api instead of trying to open a tty device node from within the kernel. I was thinking about that, but it looks tricky to use for a "normal" PC-based distro due to typical lack of device-tree support. Another possibility is to support comedi drivers in userspace via CUSE, which would require a comedi-cuse interface library that user-space comedi drivers could link to similar to how kernel-mode comedi drivers link to the comedi core. I don't object to dropping it. Ian and Hartley, do you know of any users of this driver anymore? The only place I know it was in use was in Anders Blomdell's lab at Lund University. I've added him to the Cc: list. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] Staging: comedi: ssv_dnp: fixed style line length warning
On 13/06/18 20:01, Javier Martinez wrote: Fixed style line length warning detected by checkpatch.pl in the file ssv_dnp.c. Signed-off-by: Javier Martinez --- drivers/staging/comedi/drivers/ssv_dnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/ssv_dnp.c b/drivers/staging/comedi/drivers/ssv_dnp.c index 0628060e42ca..87f46e0eb9ee 100644 --- a/drivers/staging/comedi/drivers/ssv_dnp.c +++ b/drivers/staging/comedi/drivers/ssv_dnp.c @@ -16,7 +16,7 @@ * Status: unknown */ -/* include files --- */ +/* include files -- */ #include #include "../comedidev.h" I cannot reproduce the checkpatch.pl warning. The line is only 79 columns wide. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] Staging: comedi: ssv_dnp: fixed style line length warning
On 13/06/18 20:01, Javier Martinez wrote: Fixed style line length warning detected by checkpatch.pl in the file ssv_dnp.c. Signed-off-by: Javier Martinez --- drivers/staging/comedi/drivers/ssv_dnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/ssv_dnp.c b/drivers/staging/comedi/drivers/ssv_dnp.c index 0628060e42ca..87f46e0eb9ee 100644 --- a/drivers/staging/comedi/drivers/ssv_dnp.c +++ b/drivers/staging/comedi/drivers/ssv_dnp.c @@ -16,7 +16,7 @@ * Status: unknown */ -/* include files --- */ +/* include files -- */ #include #include "../comedidev.h" I cannot reproduce the checkpatch.pl warning. The line is only 79 columns wide. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
On 13/06/18 20:05, Dan Carpenter wrote: On Wed, Jun 13, 2018 at 08:26:43PM +0200, Chris Opperman wrote: Hi Dan/Ian, Noted your comments regarding additional text, thanks! Just curious whether the "scissors" format given at the link below is valid? https://kernelnewbies.org/PatchTipsAndTricks It is given as an alternative to placing additional text below the cut-off line. Try it yourself. Save your email as email.txt and then `cat email.txt | git am` and then review the patch with git log -p. I've seen people do the scissors thing, but I assume the maintainer has to hand edit the log which we refuse to do. It can be done automatically with the git am -c or --scissors option, or by setting mailinfo.scissors to true in the git config. But since the use of these special scissors lines is not documented in Documentation/process/submitting-patches.rst, I don't think it is safe to assume that no manual intervention would be required by the maintainer to deal with it. I wonder where the "scissors" advice on the kernelnewbies page came from? -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
On 13/06/18 20:05, Dan Carpenter wrote: On Wed, Jun 13, 2018 at 08:26:43PM +0200, Chris Opperman wrote: Hi Dan/Ian, Noted your comments regarding additional text, thanks! Just curious whether the "scissors" format given at the link below is valid? https://kernelnewbies.org/PatchTipsAndTricks It is given as an alternative to placing additional text below the cut-off line. Try it yourself. Save your email as email.txt and then `cat email.txt | git am` and then review the patch with git log -p. I've seen people do the scissors thing, but I assume the maintainer has to hand edit the log which we refuse to do. It can be done automatically with the git am -c or --scissors option, or by setting mailinfo.scissors to true in the git config. But since the use of these special scissors lines is not documented in Documentation/process/submitting-patches.rst, I don't think it is safe to assume that no manual intervention would be required by the maintainer to deal with it. I wonder where the "scissors" advice on the kernelnewbies page came from? -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: add error handling for vmap
On 14/06/18 08:57, Zhouyang Jia wrote:
Hi,
I reported this bug since more than 90% callsites of vmap are
well handled in kernel. The caller function __comedi_buf_alloc
has no return value, so I don't know how the error is handled in
its caller.
I believe there would be a better error handling method, but I
have limited domain knowledge, so I'm sorry I can't help here.
Thanks for your kind reply.
Best,
Zhouyang
__comedi_buf_alloc is just a helper function factored out of, and called
by comedi_buf_alloc. __comedi_buf_alloc does not clean up after itself
on error because it expects comedi_buf_alloc to detect the error and
call __comedi_buf_free to clean up any partially allocated mess:
__comedi_buf_alloc(dev, s, n_pages);
if (!async->prealloc_buf) {
/*
* [This is not the actual comment in the code!]
*
* Error occured in __comedi_buf_alloc().
* Buffer may be partially allocated.
* Call __comedi_buf_free() to clean it up.
*/
__comedi_buf_free(dev, s);
return -ENOMEM;
}
2018-06-12 19:50 GMT+08:00 Dan Carpenter <mailto:dan.carpen...@oracle.com>>:
On Tue, Jun 12, 2018 at 11:25:35AM +0800, Zhouyang Jia wrote:
> When vmap fails, the lack of error-handling code may
> cause unexpected results.
>
> This patch adds error-handling code after calling vmap.
>
Again, this is not error handling, this is just an error message. This
error condition is handled in the caller.
regards,
dan carpenter
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH] staging: comedi: add error handling for vmap
On 14/06/18 08:57, Zhouyang Jia wrote:
Hi,
I reported this bug since more than 90% callsites of vmap are
well handled in kernel. The caller function __comedi_buf_alloc
has no return value, so I don't know how the error is handled in
its caller.
I believe there would be a better error handling method, but I
have limited domain knowledge, so I'm sorry I can't help here.
Thanks for your kind reply.
Best,
Zhouyang
__comedi_buf_alloc is just a helper function factored out of, and called
by comedi_buf_alloc. __comedi_buf_alloc does not clean up after itself
on error because it expects comedi_buf_alloc to detect the error and
call __comedi_buf_free to clean up any partially allocated mess:
__comedi_buf_alloc(dev, s, n_pages);
if (!async->prealloc_buf) {
/*
* [This is not the actual comment in the code!]
*
* Error occured in __comedi_buf_alloc().
* Buffer may be partially allocated.
* Call __comedi_buf_free() to clean it up.
*/
__comedi_buf_free(dev, s);
return -ENOMEM;
}
2018-06-12 19:50 GMT+08:00 Dan Carpenter <mailto:dan.carpen...@oracle.com>>:
On Tue, Jun 12, 2018 at 11:25:35AM +0800, Zhouyang Jia wrote:
> When vmap fails, the lack of error-handling code may
> cause unexpected results.
>
> This patch adds error-handling code after calling vmap.
>
Again, this is not error handling, this is just an error message. This
error condition is handled in the caller.
regards,
dan carpenter
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
Re: [PATCH v5] staging: comedi: Improved readability of function comedi_nsamples_left.
On 13/06/18 18:14, Chris Opperman wrote:
Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.
Signed-off-by: Chris Opperman
---
Changes v5:
a) Moved additional text to below the cut-off line.
The "Changes v5" text is a bit incomplete without the earlier change
history, but let's forget that since the previous change history was a
bit messed up anyway.
drivers/staging/comedi/drivers.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice
*s,
{
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+ unsigned long long scans_left;
+ unsigned long long samples_left;
- if (cmd->stop_src == TRIG_COUNT) {
- unsigned int scans_left = __comedi_nscans_left(s,
cmd->stop_arg);
- unsigned int scan_pos =
- comedi_bytes_to_samples(s, async->scan_progress);
- unsigned long long samples_left = 0;
-
- if (scans_left) {
- samples_left = ((unsigned long long)scans_left *
- cmd->scan_end_arg) - scan_pos;
- }
+ if (cmd->stop_src != TRIG_COUNT)
+ return nsamples;
- if (samples_left < nsamples)
- nsamples = samples_left;
- }
+ scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+ if (!scans_left)
+ return 0;
+
+ samples_left = scans_left * cmd->scan_end_arg -
+ comedi_bytes_to_samples(s, async->scan_progress);
+
+ if (samples_left < nsamples)
+ return samples_left;
return nsamples;
}
EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4
The actual patch looks fine thanks!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
