All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	Pavel Skripkin <paskripkin@gmail.com>
Cc: Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
	straube.linux@gmail.com, linux-staging@lists.linux.dev,
	linux-kernel@vger.kernel.org, Martin Kaiser <martin@kaiser.cx>
Subject: Re: [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs
Date: Sun, 22 Aug 2021 18:03:07 +0200	[thread overview]
Message-ID: <23968040.bvS6LFdsLj@localhost.localdomain> (raw)
In-Reply-To: <6182ed46-d79d-7f66-c7c0-096486410b4d@gmail.com>

On Sunday, August 22, 2021 3:31:31 PM CEST Pavel Skripkin wrote:
> On 8/22/21 4:21 PM, Fabio M. De Francesco wrote:
> > On Sunday, August 22, 2021 2:39:34 PM CEST Greg KH wrote:
> >> On Sun, Aug 22, 2021 at 03:10:56PM +0300, Pavel Skripkin wrote:
> >> > On 8/22/21 1:59 PM, Fabio M. De Francesco wrote:
> >> > > On Sunday, August 22, 2021 12:09:29 PM CEST Pavel Skripkin wrote:
> > [...]
> >> > > So, it's up to the callers to test if (!_rtw_read*()) and then act
> >> > > accordingly. If they get 0 they should know how to handle the errors.
> >> > 
> >> > Yes, but _rtw_read*() == 0 indicates 2 states:
> >> > 	1. Error on transfer side
> >> > 	2. Actual register value is 0
> >> 
> >> That's not a good design, it should be fixed.  Note there is the new
> >> usb_control_msg_recv() function which should probably be used instead
> >> here, to prevent this problem from happening.
> > 
> > I think that no functions should return 0 for signaling FAILURE. If I'm not
> > wrong, the kernel quite always prefers to return 0 on SUCCESS and <0 on
> > FAILURE. Why don't you just fix this?
> > 
> That's what I've done in v2. All rtw_read* family will have following 
> prototype in v2:
> 
> int __must_check _rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
> 
> Was it your idea, or you were talking about different approach?
> 
> With regards,
> Pavel Skripkin

Pavel,

Yes, it is correct.

However, after that I had time to look at the calls chain and understand what 
each function does and then I saw that my initial proposal should be made
along with another one...

The calls chain is:

(1)        _rtw_read8()  <--- (returns the data read from next function in chain) 
                                            (no errors returned, see possible fix in next function)
(2)                usb_read8() <--- (returns the data read from next function in chain) 
                                                 (_data_may_be_unitialised_, no errors returned)
		  (possible fix: from "u8 data"; to "char data = -1;")
(3)                           usbctrl_vendorreq() <---- (returns data read from next function in chain)
		                            (data is always a valid pointer saved to third argument)
		                            (if it fails, the third argument is unchanged because it
		                            still has the address of the "data" argument given by the caller)
(4)	                usb_control_msg() <---- (it always returns how many bytes read or valid error codes)
		                                  (it _never_ returns 0: either positive or negative values)
		
I have not yet looked at the usb_control_msg_recv() which Greg talked about.

To summarize: in function (2) "u8 data" should become "char data = -1;".

Regards,

Fabio

P.S.: I was about to send this message while I see that you sent v2. Since I've already have
this response to your question I send it and soon after I'm going to read your v2 patches. 
 





  parent reply	other threads:[~2021-08-22 16:03 UTC|newest]

Thread overview: 140+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 17:07 [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs Pavel Skripkin
2021-08-20 17:07 ` [PATCH RFC 1/3] staging: r8188eu: add proper rtw_read* error handling Pavel Skripkin
2021-08-20 20:54   ` kernel test robot
2021-08-20 21:50   ` Pavel Skripkin
2021-08-20 23:41   ` Phillip Potter
2021-08-20 23:41     ` Phillip Potter
2021-08-21  5:55   ` Fabio M. De Francesco
2021-08-21 10:35     ` Pavel Skripkin
2021-08-21 12:11       ` Fabio M. De Francesco
2021-08-20 17:07 ` [PATCH RFC 2/3] staging: r8188eu: add error handling to ReadFuse Pavel Skripkin
2021-08-20 23:51   ` Phillip Potter
2021-08-20 23:51     ` Phillip Potter
2021-08-21  3:59     ` Fabio M. De Francesco
2021-08-20 17:07 ` [PATCH RFC 3/3] staging: r8188eu: add error argument to read_macreg Pavel Skripkin
2021-08-20 23:18   ` Phillip Potter
2021-08-20 23:18     ` Phillip Potter
2021-08-21 10:38     ` Pavel Skripkin
2021-08-20 23:12 ` [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs Phillip Potter
2021-08-20 23:12   ` Phillip Potter
2021-08-21 10:42   ` Pavel Skripkin
2021-08-22  9:53 ` Fabio M. De Francesco
2021-08-22 10:09   ` Pavel Skripkin
2021-08-22 10:59     ` Fabio M. De Francesco
2021-08-22 11:34       ` Fabio M. De Francesco
2021-08-22 12:10       ` Pavel Skripkin
2021-08-22 12:39         ` Greg KH
2021-08-22 12:50           ` Pavel Skripkin
2021-08-22 13:06             ` Greg KH
2021-08-22 13:21           ` Fabio M. De Francesco
2021-08-22 13:30             ` Greg KH
2021-08-22 13:31             ` Pavel Skripkin
2021-08-22 14:35               ` [PATCH RFC v2 0/6] " Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 1/6] staging: r8188eu: remove {read,write}_macreg Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 2/6] staging: r8188eu: add helper macro for printing registers Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 3/6] staging: r8188eu: add error handling of rtw_read8 Pavel Skripkin
2021-08-22 16:49                   ` kernel test robot
2021-08-22 14:35                 ` [PATCH RFC v2 4/6] staging: r8188eu: add error handling of rtw_read16 Pavel Skripkin
2021-08-22 14:36                 ` [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32 Pavel Skripkin
2021-08-23 23:33                   ` Phillip Potter
2021-08-23 23:33                     ` Phillip Potter
2021-08-24  0:10                     ` Fabio M. De Francesco
2021-08-24  6:40                       ` Pavel Skripkin
2021-08-24  8:38                         ` Fabio M. De Francesco
2021-08-24  8:47                           ` Pavel Skripkin
2021-08-24  8:53                             ` Pavel Skripkin
2021-08-24  9:46                               ` Fabio M. De Francesco
2021-08-24 22:10                               ` Phillip Potter
2021-08-24 22:10                                 ` Phillip Potter
2021-08-24 22:07                             ` Phillip Potter
2021-08-24 22:07                               ` Phillip Potter
2021-08-24  6:53                     ` Pavel Skripkin
2021-08-24  7:25                     ` [PATCH v3 0/6] staging: r8188eu: avoid uninit value bugs Pavel Skripkin
2021-08-24  7:27                       ` [PATCH v3 1/6] staging: r8188eu: remove {read,write}_macreg Pavel Skripkin
2021-08-26 10:39                         ` Greg KH
2021-08-26 10:40                         ` Greg KH
2021-08-24  7:27                       ` [PATCH v3 2/6] staging: r8188eu: add helper macro for printing registers Pavel Skripkin
2021-08-26 10:37                         ` Greg KH
2021-08-24  7:27                       ` [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8 Pavel Skripkin
2021-08-25 12:05                         ` kernel test robot
2021-08-25 12:05                           ` kernel test robot
2021-08-25 12:17                           ` Pavel Skripkin
2021-08-25 12:17                             ` Pavel Skripkin
2021-08-25 12:51                             ` Dan Carpenter
2021-08-25 12:51                               ` Dan Carpenter
2021-08-25 13:02                               ` Pavel Skripkin
2021-08-25 13:02                                 ` Pavel Skripkin
2021-08-25 13:34                                 ` Dan Carpenter
2021-08-25 13:34                                   ` Dan Carpenter
2021-08-25 13:44                                   ` Pavel Skripkin
2021-08-25 13:44                                     ` Pavel Skripkin
2021-08-25 17:11                                     ` Nick Desaulniers
2021-08-25 17:11                                       ` Nick Desaulniers
2021-08-25 17:11                                       ` Nick Desaulniers
2021-08-26 11:08                                       ` Dan Carpenter
2021-08-26 11:08                                         ` Dan Carpenter
2021-08-25 23:45                         ` Fabio M. De Francesco
2021-08-26  5:13                           ` Pavel Skripkin
2021-08-26  8:21                         ` David Laight
2021-08-26  8:27                           ` Pavel Skripkin
2021-08-26 10:19                             ` David Laight
2021-08-26 11:21                               ` Dan Carpenter
2021-08-27  8:14                                 ` David Laight
2021-08-27  8:22                                   ` Pavel Skripkin
2021-08-27  9:07                         ` Dan Carpenter
2021-08-27  9:16                           ` Pavel Skripkin
2021-08-27  9:23                             ` Dan Carpenter
2021-08-30 11:21                         ` kernel test robot
2021-08-30 11:21                           ` kernel test robot
2021-08-24  7:27                       ` [PATCH v3 4/6] staging: r8188eu: add error handling of rtw_read16 Pavel Skripkin
2021-08-25  4:35                         ` Fabio M. De Francesco
2021-08-25  8:22                           ` Pavel Skripkin
2021-08-25  9:48                             ` Fabio M. De Francesco
2021-08-25  9:55                               ` Pavel Skripkin
2021-08-25 10:06                                 ` Dan Carpenter
2021-08-25 10:13                                   ` Pavel Skripkin
2021-08-25 10:38                                     ` Dan Carpenter
2021-08-25 10:41                                       ` Pavel Skripkin
2021-08-25 11:06                                       ` Fabio M. De Francesco
2021-08-25 11:11                                         ` Fabio M. De Francesco
2021-08-25 11:31                                         ` Dan Carpenter
2021-08-25 12:11                                           ` Fabio M. De Francesco
2021-08-25 10:51                                 ` Fabio M. De Francesco
2021-08-26 10:50                         ` Greg KH
2021-08-26 10:58                           ` Pavel Skripkin
2021-08-24  7:27                       ` [PATCH v3 5/6] staging: r8188eu: add error handling of rtw_read32 Pavel Skripkin
2021-08-25  4:40                         ` Fabio M. De Francesco
2021-08-26  8:51                         ` David Laight
2021-08-26  9:22                           ` Pavel Skripkin
2021-08-26  9:27                             ` Pavel Skripkin
2021-08-26 10:22                               ` David Laight
2021-08-26 10:55                                 ` Pavel Skripkin
2021-08-26 10:59                                   ` David Laight
2021-08-26 20:03                                     ` Pavel Skripkin
2021-08-27  7:12                                       ` gregkh
2021-08-27  7:16                                         ` Pavel Skripkin
2021-08-24  7:27                       ` [PATCH v3 6/6] staging: r8188eu: make ReadEFuse return an int Pavel Skripkin
2021-08-25 10:13                       ` [PATCH v3 0/6] staging: r8188eu: avoid uninit value bugs Fabio M. De Francesco
2021-08-27  7:49                       ` Kari Argillander
2021-08-27  7:52                         ` Pavel Skripkin
2021-08-24  6:58                   ` [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32 Dan Carpenter
2021-08-24  7:01                     ` Pavel Skripkin
2021-08-24 15:07                       ` Fabio M. De Francesco
2021-08-22 14:36                 ` [PATCH RFC v2 6/6] staging: r8188eu: make ReadEFuse return an int Pavel Skripkin
2021-08-22 15:30                 ` [PATCH RFC v2 0/6] staging: r8188eu: avoid uninit value bugs Pavel Skripkin
2021-08-22 16:05                   ` Michael Straube
2021-08-22 16:26                     ` Pavel Skripkin
2021-08-22 23:52                       ` Phillip Potter
2021-08-22 23:52                         ` Phillip Potter
2021-08-22 17:36                 ` Fabio M. De Francesco
2021-08-22 17:38                   ` Pavel Skripkin
2021-08-22 20:06                     ` Fabio M. De Francesco
2021-08-22 20:19                       ` Pavel Skripkin
2021-08-23  0:12                 ` Phillip Potter
2021-08-23  0:12                   ` Phillip Potter
2021-08-23  6:38                   ` Pavel Skripkin
2021-08-23  6:44                   ` Pavel Skripkin
2021-08-22 16:03               ` Fabio M. De Francesco [this message]
2021-08-22 16:15                 ` [PATCH RFC 0/3] " Pavel Skripkin
2021-08-22 15:04             ` Phillip Potter
2021-08-22 15:04               ` Phillip Potter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=23968040.bvS6LFdsLj@localhost.localdomain \
    --to=fmdefrancesco@gmail.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=martin@kaiser.cx \
    --cc=paskripkin@gmail.com \
    --cc=phil@philpotter.co.uk \
    --cc=straube.linux@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.