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 <[email protected]>
---
v3:
- using 'goto err' rather than 'buf_len=-EOVERFLOW'.(Thanks to Sasha)
v2:
- split v1 patch to different patches
https://lore.kernel.org/all/[email protected]/
v1:
https://lore.kernel.org/all/[email protected]/
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
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 <[email protected]>
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/[email protected]/
>
> v1:
> https://lore.kernel.org/all/[email protected]/
>
> 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
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
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
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/
> >>>> 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