[PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[Su Hui]

Clang static analyzer complains that value stored to 'rets' is never
read.Using 'goto err' to go to the error path and fix this problem.

Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
v3: 
 - using 'goto err' rather than 'buf_len=-EOVERFLOW'.(Thanks to Sasha)

v2:
 - split v1 patch to different patches
https://lore.kernel.org/all/20231120095523.178385-2-suhui@nfschina.com/

v1:
https://lore.kernel.org/all/5c98fc07-36a9-92cc-f8d6-c4efdc0c34aa@nfschina.com/

 drivers/misc/mei/client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 7ea80779a0e2..0489bec4fded 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -2033,7 +2033,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
 	hbuf_slots = mei_hbuf_empty_slots(dev);
 	if (hbuf_slots < 0) {
 		rets = -EOVERFLOW;
-		goto out;
+		goto err;
 	}
 
 	hbuf_len = mei_slots2data(hbuf_slots) & MEI_MSG_MAX_LEN_MASK;
-- 
2.30.2

Re: [PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[Greg KH]

On Tue, Nov 28, 2023 at 09:45:08AM +0800, Su Hui wrote:
> Clang static analyzer complains that value stored to 'rets' is never
> read.Using 'goto err' to go to the error path and fix this problem.
> 
> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
> Signed-off-by: Su Hui <suhui@nfschina.com>

How was this tested?

> ---
> v3: 
>  - using 'goto err' rather than 'buf_len=-EOVERFLOW'.(Thanks to Sasha)
> 
> v2:
>  - split v1 patch to different patches
> https://lore.kernel.org/all/20231120095523.178385-2-suhui@nfschina.com/
> 
> v1:
> https://lore.kernel.org/all/5c98fc07-36a9-92cc-f8d6-c4efdc0c34aa@nfschina.com/
> 
>  drivers/misc/mei/client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 7ea80779a0e2..0489bec4fded 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -2033,7 +2033,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>  	hbuf_slots = mei_hbuf_empty_slots(dev);
>  	if (hbuf_slots < 0) {
>  		rets = -EOVERFLOW;
> -		goto out;
> +		goto err;

Please prove that this is correct, as based on the code logic, it seems
very wrong.  I can't take this unless the code is tested properly.

thanks,

greg k-h

Re: [PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[Dan Carpenter]

On Mon, Dec 04, 2023 at 09:00:42AM +0100, Greg KH wrote:
> > diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> > index 7ea80779a0e2..0489bec4fded 100644
> > --- a/drivers/misc/mei/client.c
> > +++ b/drivers/misc/mei/client.c
> > @@ -2033,7 +2033,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
> >  	hbuf_slots = mei_hbuf_empty_slots(dev);
> >  	if (hbuf_slots < 0) {
> >  		rets = -EOVERFLOW;
> > -		goto out;
> > +		goto err;
> 
> Please prove that this is correct, as based on the code logic, it seems
> very wrong.  I can't take this unless the code is tested properly.

Hi Greg,

When Su Hui sent the v2 patch you sent an auto response about adding
stable to the CC list.
https://lore.kernel.org/all/2023112042-napped-snoring-b766@gregkh/

However, it appears that you still applied the v2 patch.  It's in
linux-next as commit ee6236027218 ("misc: mei: client.c: fix problem of
return '-EOVERFLOW' in mei_cl_write").

When I use `git am` to apply this patch, then it doesn't apply.  However,
when I use cat email.txt | patch -p1 then it tries to reverse the patch
and apply it to a different function.

So this a kind of confusing thing.

regards,
dan carpenter

Re: [PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[Greg KH]

On Mon, Dec 04, 2023 at 04:11:31PM +0300, Dan Carpenter wrote:
> On Mon, Dec 04, 2023 at 09:00:42AM +0100, Greg KH wrote:
> > > diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> > > index 7ea80779a0e2..0489bec4fded 100644
> > > --- a/drivers/misc/mei/client.c
> > > +++ b/drivers/misc/mei/client.c
> > > @@ -2033,7 +2033,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
> > >  	hbuf_slots = mei_hbuf_empty_slots(dev);
> > >  	if (hbuf_slots < 0) {
> > >  		rets = -EOVERFLOW;
> > > -		goto out;
> > > +		goto err;
> > 
> > Please prove that this is correct, as based on the code logic, it seems
> > very wrong.  I can't take this unless the code is tested properly.
> 
> Hi Greg,
> 
> When Su Hui sent the v2 patch you sent an auto response about adding
> stable to the CC list.
> https://lore.kernel.org/all/2023112042-napped-snoring-b766@gregkh/
> 
> However, it appears that you still applied the v2 patch.  It's in
> linux-next as commit ee6236027218 ("misc: mei: client.c: fix problem of
> return '-EOVERFLOW' in mei_cl_write").
> 
> When I use `git am` to apply this patch, then it doesn't apply.  However,
> when I use cat email.txt | patch -p1 then it tries to reverse the patch
> and apply it to a different function.

Odd, I missed that I had already applied the first one, nevermind, that
one is correct, this one was wrong :)

thanks,

greg k-h

Re: [PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[Su Hui]

On 2023/12/4 21:17, Greg KH wrote:
> On Mon, Dec 04, 2023 at 04:11:31PM +0300, Dan Carpenter wrote:
>> On Mon, Dec 04, 2023 at 09:00:42AM +0100, Greg KH wrote:
>>>> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
>>>> index 7ea80779a0e2..0489bec4fded 100644
>>>> --- a/drivers/misc/mei/client.c
>>>> +++ b/drivers/misc/mei/client.c
>>>> @@ -2033,7 +2033,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>>>>   	hbuf_slots = mei_hbuf_empty_slots(dev);
>>>>   	if (hbuf_slots < 0) {
>>>>   		rets = -EOVERFLOW;
>>>> -		goto out;
>>>> +		goto err;
>>> Please prove that this is correct, as based on the code logic, it seems
>>> very wrong.  I can't take this unless the code is tested properly.
>> Hi Greg,
>>
>> When Su Hui sent the v2 patch you sent an auto response about adding
>> stable to the CC list.
>> https://lore.kernel.org/all/2023112042-napped-snoring-b766@gregkh/
>>
>> However, it appears that you still applied the v2 patch.  It's in
>> linux-next as commit ee6236027218 ("misc: mei: client.c: fix problem of
>> return '-EOVERFLOW' in mei_cl_write").
>>
>> When I use `git am` to apply this patch, then it doesn't apply.  However,
>> when I use cat email.txt | patch -p1 then it tries to reverse the patch
>> and apply it to a different function.
> Odd, I missed that I had already applied the first one, nevermind, that
> one is correct, this one was wrong :)

Hi,

Oh, sorry...
I'm not familiar with mei device, I send this v3 patch because of Sasha' 
s suggestion.[1]
Could Sasha give some advice about this ?
Thanks a lot :)

https://lore.kernel.org/all/CY5PR11MB63668F464A281A239FA12B6AEDBDA@CY5PR11MB6366.namprd11.prod.outlook.com/

RE: [PATCH v3] misc: mei: client.c: fix problem of return '-EOVERFLOW' in mei_cl_write

[Usyskin, Alexander]

> >>>> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> >>>> index 7ea80779a0e2..0489bec4fded 100644
> >>>> --- a/drivers/misc/mei/client.c
> >>>> +++ b/drivers/misc/mei/client.c
> >>>> @@ -2033,7 +2033,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct
> mei_cl_cb *cb, unsigned long time
> >>>>   	hbuf_slots = mei_hbuf_empty_slots(dev);
> >>>>   	if (hbuf_slots < 0) {
> >>>>   		rets = -EOVERFLOW;
> >>>> -		goto out;
> >>>> +		goto err;
> >>> Please prove that this is correct, as based on the code logic, it seems
> >>> very wrong.  I can't take this unless the code is tested properly.
> >> Hi Greg,
> >>
> >> When Su Hui sent the v2 patch you sent an auto response about adding
> >> stable to the CC list.
> >> https://lore.kernel.org/all/2023112042-napped-snoring-b766@gregkh/
> >>
> >> However, it appears that you still applied the v2 patch.  It's in
> >> linux-next as commit ee6236027218 ("misc: mei: client.c: fix problem of
> >> return '-EOVERFLOW' in mei_cl_write").
> >>
> >> When I use `git am` to apply this patch, then it doesn't apply.  However,
> >> when I use cat email.txt | patch -p1 then it tries to reverse the patch
> >> and apply it to a different function.
> > Odd, I missed that I had already applied the first one, nevermind, that
> > one is correct, this one was wrong :)
> 
> Hi,
> 
> Oh, sorry...
> I'm not familiar with mei device, I send this v3 patch because of Sasha'
> s suggestion.[1]
> Could Sasha give some advice about this ?
> Thanks a lot :)
> 
> https://lore.kernel.org/all/CY5PR11MB63668F464A281A239FA12B6AEDBDA@C
> Y5PR11MB6366.namprd11.prod.outlook.com/
> 

When we have overflow from the hbuf - that means the HW bookkeeping
is in mess (write pointer is before read pointer).
There is no point in continuing in the write flow, we should exit with error and
let driver to re-initialize HW.
I've never seen this error in the wild, so it is very hard to test it.

-- 
Thanks,
Sasha