Re: [PATCH] Staging: comedi: clean up conditional statement in addi_apci_3xxx.c
On 2014-02-13 10:08, Ian Abbott wrote: On 2014-02-13 03:29, Chase Southwood wrote: In this if-else conditional statement, if (chan 16), but (data[0] == INSN_CONFIG_DIO_QUERY), the function does not return early, but the else-branch does not get executed either. As a result, mask would be used uninitialized in the next line. What we want here is if (chan 16) and (data[0] != INSN_CONFIG_DIO_QUERY), return an error, but in every other case, initialize mask and then proceed. Found by a static checker. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- drivers/staging/comedi/drivers/addi_apci_3xxx.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c b/drivers/staging/comedi/drivers/addi_apci_3xxx.c index ceadf8e..04c5153 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c +++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c @@ -688,13 +688,11 @@ static int apci3xxx_dio_insn_config(struct comedi_device *dev, * Port 1 (channels 8-15) are always outputs * Port 2 (channels 16-23) are programmable i/o */ -if (chan 16) { -if (data[0] != INSN_CONFIG_DIO_QUERY) -return -EINVAL; -} else { -/* changing any channel in port 2 changes the entire port */ -mask = 0xff; -} +if ((chan 16) (data[0] != INSN_CONFIG_DIO_QUERY)) +return -EINVAL; + +/* changing any channel in port 2 changes the entire port */ +mask = 0xff; ret = comedi_dio_insn_config(dev, s, insn, data, mask); if (ret) Seems correct. Acked-by: Ian Abbott abbo...@mev.co.uk Okay, as Hartley pointed out, it isn't correct, so I withdraw my Acked-by. -- -=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] Staging: comedi: clean up conditional statement in addi_apci_3xxx.c
On Friday, February 14, 2014 4:50 AM, Ian Abbott wrote: On 2014-02-13 18:25, Hartley Sweeten wrote: On Wednesday, February 12, 2014 8:29 PM, Chase Southwood wrote: In this if-else conditional statement, if (chan 16), but (data[0] == INSN_CONFIG_DIO_QUERY), the function does not return early, but the else-branch does not get executed either. As a result, mask would be used uninitialized in the next line. What we want here is if (chan 16) and (data[0] != INSN_CONFIG_DIO_QUERY), return an error, but in every other case, initialize mask and then proceed. Found by a static checker. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- drivers/staging/comedi/drivers/addi_apci_3xxx.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c b/drivers/staging/comedi/drivers/addi_apci_3xxx.c index ceadf8e..04c5153 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c +++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c @@ -688,13 +688,11 @@ static int apci3xxx_dio_insn_config(struct comedi_device *dev, * Port 1 (channels 8-15) are always outputs * Port 2 (channels 16-23) are programmable i/o */ - if (chan 16) { - if (data[0] != INSN_CONFIG_DIO_QUERY) - return -EINVAL; - } else { - /* changing any channel in port 2 changes the entire port */ - mask = 0xff; - } + if ((chan 16) (data[0] != INSN_CONFIG_DIO_QUERY)) + return -EINVAL; + + /* changing any channel in port 2 changes the entire port */ + mask = 0xff; ret = comedi_dio_insn_config(dev, s, insn, data, mask); if (ret) The uninitialized mask when chan 16 is an issue. But your patch is not quite correct. The original code was intending to limit the valid instructions for channels 16 to only INSN_CONFIG_DIO_QUERY. These channels have fixed directions: 0-7 (port 0) are always inputs and 8-15 (port 1) are always outputs. Channels 16-23 (port 2) have programmable direction but changing any channel effects the entire port, that's what the 0xff mask is for. Changing the mask to 0xff for any chanspec will result in the INSN_CONFIG_DIO_QUERY instruction returning the direction of port 2 regardless of what the chanspec is. The right fix would be: 1) Default the mask to 0 so that comedi_dio_insn_config() will use a chan_mask based on the chanspec for the INSN_CONFIG_DIO_QUERY instruction. 2) Ignore all instructions except INSN_CONFIG_DIO_QUERY when the chan 16. 3) Modify the mask for chan = 16 when the instruction is not INSN_CONFIG_DIO_QUERY so that the INSN_CONFIG_DIO_{INPUT,OUTPUT} instructions update the entire port. Agreed, but another question is why does comedi_dio_insn_config() in drivers.c need to look at the supplied mask at all for INSN_CONFIG_DIO_QUERY? It doesn't, that's why the mask is set to a default of 0 (1 above) and only modified (3 above) when the instruction is not INSN_CONFIG_DIO_QUERY and chan = 16. With a mask of 0 comedi_dio_insn_config() will properly figure out the mask based on the chanspec. Regards, Hartley ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: comedi: clean up conditional statement in addi_apci_3xxx.c
On 2014-02-13 03:29, Chase Southwood wrote: In this if-else conditional statement, if (chan 16), but (data[0] == INSN_CONFIG_DIO_QUERY), the function does not return early, but the else-branch does not get executed either. As a result, mask would be used uninitialized in the next line. What we want here is if (chan 16) and (data[0] != INSN_CONFIG_DIO_QUERY), return an error, but in every other case, initialize mask and then proceed. Found by a static checker. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- drivers/staging/comedi/drivers/addi_apci_3xxx.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c b/drivers/staging/comedi/drivers/addi_apci_3xxx.c index ceadf8e..04c5153 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c +++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c @@ -688,13 +688,11 @@ static int apci3xxx_dio_insn_config(struct comedi_device *dev, * Port 1 (channels 8-15) are always outputs * Port 2 (channels 16-23) are programmable i/o */ - if (chan 16) { - if (data[0] != INSN_CONFIG_DIO_QUERY) - return -EINVAL; - } else { - /* changing any channel in port 2 changes the entire port */ - mask = 0xff; - } + if ((chan 16) (data[0] != INSN_CONFIG_DIO_QUERY)) + return -EINVAL; + + /* changing any channel in port 2 changes the entire port */ + mask = 0xff; ret = comedi_dio_insn_config(dev, s, insn, data, mask); if (ret) Seems correct. Acked-by: Ian Abbott abbo...@mev.co.uk -- -=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] Staging: comedi: clean up conditional statement in addi_apci_3xxx.c
On Wednesday, February 12, 2014 8:29 PM, Chase Southwood wrote: In this if-else conditional statement, if (chan 16), but (data[0] == INSN_CONFIG_DIO_QUERY), the function does not return early, but the else-branch does not get executed either. As a result, mask would be used uninitialized in the next line. What we want here is if (chan 16) and (data[0] != INSN_CONFIG_DIO_QUERY), return an error, but in every other case, initialize mask and then proceed. Found by a static checker. Signed-off-by: Chase Southwood chase.southw...@yahoo.com --- drivers/staging/comedi/drivers/addi_apci_3xxx.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c b/drivers/staging/comedi/drivers/addi_apci_3xxx.c index ceadf8e..04c5153 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c +++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c @@ -688,13 +688,11 @@ static int apci3xxx_dio_insn_config(struct comedi_device *dev, * Port 1 (channels 8-15) are always outputs * Port 2 (channels 16-23) are programmable i/o */ - if (chan 16) { - if (data[0] != INSN_CONFIG_DIO_QUERY) - return -EINVAL; - } else { - /* changing any channel in port 2 changes the entire port */ - mask = 0xff; - } + if ((chan 16) (data[0] != INSN_CONFIG_DIO_QUERY)) + return -EINVAL; + + /* changing any channel in port 2 changes the entire port */ + mask = 0xff; ret = comedi_dio_insn_config(dev, s, insn, data, mask); if (ret) The uninitialized mask when chan 16 is an issue. But your patch is not quite correct. The original code was intending to limit the valid instructions for channels 16 to only INSN_CONFIG_DIO_QUERY. These channels have fixed directions: 0-7 (port 0) are always inputs and 8-15 (port 1) are always outputs. Channels 16-23 (port 2) have programmable direction but changing any channel effects the entire port, that's what the 0xff mask is for. Changing the mask to 0xff for any chanspec will result in the INSN_CONFIG_DIO_QUERY instruction returning the direction of port 2 regardless of what the chanspec is. The right fix would be: 1) Default the mask to 0 so that comedi_dio_insn_config() will use a chan_mask based on the chanspec for the INSN_CONFIG_DIO_QUERY instruction. 2) Ignore all instructions except INSN_CONFIG_DIO_QUERY when the chan 16. 3) Modify the mask for chan = 16 when the instruction is not INSN_CONFIG_DIO_QUERY so that the INSN_CONFIG_DIO_{INPUT,OUTPUT} instructions update the entire port. Something like the following patch is more correct. The comments are added just for clarity. Regards, Hartley diff --git a/drivers/staging/comedi/drivers/addi_apci_3xxx.c b/drivers/staging/comedi/drivers/addi_apci_3xxx.c index 88d14a9..6bc8b26 100644 --- a/drivers/staging/comedi/drivers/addi_apci_3xxx.c +++ b/drivers/staging/comedi/drivers/addi_apci_3xxx.c @@ -706,7 +706,7 @@ static int apci3xxx_dio_insn_config(struct comedi_device *dev, unsigned int *data) { unsigned int chan = CR_CHAN(insn-chanspec); - unsigned int mask; + unsigned int mask = 0; /* use chan_mask in comedi_dio_insn_config() */ int ret; /* @@ -714,11 +714,16 @@ static int apci3xxx_dio_insn_config(struct comedi_device *dev, * Port 1 (channels 8-15) are always outputs * Port 2 (channels 16-23) are programmable i/o */ - if (chan 16) { - if (data[0] != INSN_CONFIG_DIO_QUERY) + if (data[0] != INSN_CONFIG_DIO_QUERY) { + /* ignore all other instructions to ports 0 and 1 */ + if (chan 16) return -EINVAL; - } else { - /* changing any channel in port 2 changes the entire port */ + + /* +* Changing any channel in port 2 changes the entire port. +* Pass a custom mask to comedi_dio_insn_config() so all +* the io_bits are modified for port 2. +*/ mask = 0xff; } ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel