[PATCH] char: pcmcia: remove set but not used variable 'tmp'

[PATCH] char: pcmcia: remove set but not used variable 'tmp'

[Yu Kuai]

Fixes gcc '-Wunused-but-set-variable' warning:

drivers/char/pcmcia/cm4000_cs.c:1053:16: warning: variable ‘tmp’
set but not used [-Wunused-but-set-variable]

It is never used and so can be removed.

Fixes: c1986ee9bea3 ("[PATCH] New Omnikey Cardman 4000 driver")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/char/pcmcia/cm4000_cs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 89681f07bc78..9ec25735425d 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -1050,7 +1050,6 @@ static ssize_t cmm_write(struct file *filp, const char __user *buf,
 	struct cm4000_dev *dev = filp->private_data;
 	unsigned int iobase = dev->p_dev->resource[0]->start;
 	unsigned short s;
-	unsigned char tmp;
 	unsigned char infolen;
 	unsigned char sendT0;
 	unsigned short nsend;
@@ -1148,7 +1147,7 @@ static ssize_t cmm_write(struct file *filp, const char __user *buf,
 	set_cardparameter(dev);
 
 	/* dummy read, reset flag procedure received */
-	tmp = inb(REG_FLAGS1(iobase));
+	inb(REG_FLAGS1(iobase));
 
 	dev->flags1 = 0x20	/* T_Active */
 	    | (sendT0)
-- 
2.25.4

Re: [PATCH] char: pcmcia: remove set but not used variable 'tmp'

[Arnd Bergmann]

On Fri, May 14, 2021 at 8:21 AM Yu Kuai <yukuai3@huawei.com> wrote:
>
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/char/pcmcia/cm4000_cs.c:1053:16: warning: variable ‘tmp’
> set but not used [-Wunused-but-set-variable]
>
> It is never used and so can be removed.
>
> Fixes: c1986ee9bea3 ("[PATCH] New Omnikey Cardman 4000 driver")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good to me. This was likely written that way at a time when some
architecture implemented inb() as a macro, and ignoring its value
would cause a different warning.

Since you are already touching this file, can you have a look at this
warning as well:

   drivers/char/pcmcia/cm4000_cs.c: In function 'set_protocol':
>> drivers/char/pcmcia/cm4000_cs.c:569:16: warning: iteration 4 invokes undefined behavior [-Waggressive-loop-optimizations]
     569 |   pts_reply[i] = inb(REG_BUF_DATA(iobase));
         |   ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/char/pcmcia/cm4000_cs.c:567:2: note: within this loop
     567 |  for (i = 0; i < num_bytes_read; i++) {

This looks like a preexisting problem that was uncovered by a patch
that is now in linux-next to change the inb() definition once more,
I got a report from the kernel build bot about it after I merged the
patch into the asm-generic tree. It needs a range check on
num_bytes_read, or a Kconfig check to ensure it is not built on
architectures without working inb()/outb() operations.

        Arnd

Re: [PATCH] char: pcmcia: remove set but not used variable 'tmp'

[yukuai (C)]

On 2021/05/14 14:28, Arnd Bergmann wrote:
> On Fri, May 14, 2021 at 8:21 AM Yu Kuai <yukuai3@huawei.com> wrote:
>>
>> Fixes gcc '-Wunused-but-set-variable' warning:
>>
>> drivers/char/pcmcia/cm4000_cs.c:1053:16: warning: variable ‘tmp’
>> set but not used [-Wunused-but-set-variable]
>>
>> It is never used and so can be removed.
>>
>> Fixes: c1986ee9bea3 ("[PATCH] New Omnikey Cardman 4000 driver")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> 
> Looks good to me. This was likely written that way at a time when some
> architecture implemented inb() as a macro, and ignoring its value
> would cause a different warning.
> 
> Since you are already touching this file, can you have a look at this
> warning as well:
> 
>     drivers/char/pcmcia/cm4000_cs.c: In function 'set_protocol':
>>> drivers/char/pcmcia/cm4000_cs.c:569:16: warning: iteration 4 invokes undefined behavior [-Waggressive-loop-optimizations]
>       569 |   pts_reply[i] = inb(REG_BUF_DATA(iobase));
>           |   ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/char/pcmcia/cm4000_cs.c:567:2: note: within this loop
>       567 |  for (i = 0; i < num_bytes_read; i++) {
> 
> This looks like a preexisting problem that was uncovered by a patch
> that is now in linux-next to change the inb() definition once more,
> I got a report from the kernel build bot about it after I merged the
> patch into the asm-generic tree. It needs a range check on
> num_bytes_read, or a Kconfig check to ensure it is not built on
> architectures without working inb()/outb() operations.
> 
>          Arnd
> .
> 
Hi,

I'm not familar with the logical here, however, if io_read_num_rec_bytes
may get 'num_bytes_read' greater than 4, this loop will cause index out
of boundary. It make sense to me to add a range check. Futhermore, since
'num_bytes_read' is ensure to >= 4，I think we can change the loop to:

for (i = 0; i < 4; ++i) {
	xoutb(i, REG_BUF_ADDR(iobase));
	pts_reply[i] = inb(REG_BUF_DATA(iobase));
}

Thanks
Yu Kuai

Re: [PATCH] char: pcmcia: remove set but not used variable 'tmp'

[Arnd Bergmann]

On Sat, May 15, 2021 at 9:15 AM yukuai (C) <yukuai3@huawei.com> wrote:
> On 2021/05/14 14:28, Arnd Bergmann wrote:
>
> I'm not familar with the logical here, however, if io_read_num_rec_bytes
> may get 'num_bytes_read' greater than 4, this loop will cause index out
> of boundary. It make sense to me to add a range check. Futhermore, since
> 'num_bytes_read' is ensure to >= 4，I think we can change the loop to:
>
> for (i = 0; i < 4; ++i) {
>         xoutb(i, REG_BUF_ADDR(iobase));
>         pts_reply[i] = inb(REG_BUF_DATA(iobase));
> }

Obviously it still has to use the num_bytes_read value to determine
how many bytes were actually received, but there should be an
added cap to tell the compiler that this is never more than 4.

       Arnd