Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8

[Dan Carpenter]

On Thu, Aug 26, 2021 at 10:19:40AM +0000, David Laight wrote:
> From: Pavel Skripkin
> > Sent: 26 August 2021 09:28
> > 
> > On Thu, 26 Aug 2021 08:21:34 +0000
> > David Laight <David.Laight@xxxxxxxxxx> wrote:
> > 
> > > From: Pavel Skripkin
> > > > Sent: 24 August 2021 08:27
> > > >
> > > > _rtw_read8 function can fail in case of usb transfer failure. But
> > > > previous function prototype wasn't designed to return an error to
> > > > caller. It can cause a lot uninit value bugs all across the driver
> > > > code, since rtw_read8() returns local stack variable to caller.
> > > >
> > > > Fix it by changing the prototype of this function. Now it returns an
> > > > int: 0 on success, negative error value on failure and callers
> > > > should pass the pointer to storage location for register value.
> > >
> > > ...
> > > > +		len += snprintf(page + len, count - len,
> > > > "rtw_read8(0x%x)=0x%x\n",
> > > > +				proc_get_read_addr, (u8) tmp);
> > >
> > > That is broken.
> > >
> > 
> > Don't get it, sorry. Previous code did exactly the same thing, but
> > didn't check if read() was successful.
> 
> Look up the return value of snprintf().
> 

It's hard to understand what you are saying.  I think you are confusing
libc snprintf with the kernel snprintf?  In libc the snprintf function
can return negatives but in the kernel it cannot.  This is not going
to change.  Any code which checks for negative snprintf returns in the
kernel is wrong and should be fixed.

Anyway, the code works fine.  snprintf here is going to return a number
between 18-32 range.  It's not going to overflow the PAGE_SIZE buffer.

It would be nicer to use scnprint() but that's an unrelated patch.

Also this is all dead code.  It needs to be converted to sysfs.  The
caller is ifdeffed out because it doesn't compile.

regards,
dan carpenter