On Sun, Mar 06, 2022 at 06:50:33PM +0100, Jakob Koschel wrote:
> To move the list iterator variable into the list_for_each_entry_*()
> macro in the future it should be avoided to use the list iterator
> variable after the loop body.
>
> To *never* use the list iterator variable after the loop it was
> concluded to use a separate iterator variable [1].
>
> This removes the need to check the rc value to determine if the
> break/goto was hit and can be made more obvious
> by checking if the variable was set within the list traversal loop.
>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> drivers/usb/gadget/udc/dummy_hcd.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index a2d956af42a2..f21944707707 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -751,7 +751,7 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> struct dummy *dum;
> int retval = -EINVAL;
> unsigned long flags;
> - struct dummy_request *req = NULL;
> + struct dummy_request *req = NULL, *tmp;
>
> if (!_ep || !_req)
> return retval;
> @@ -763,17 +763,18 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>
> local_irq_save(flags);
> spin_lock(&dum->lock);
> - list_for_each_entry(req, &ep->queue, queue) {
> - if (&req->req == _req) {
> - list_del_init(&req->queue);
> + list_for_each_entry(tmp, &ep->queue, queue) {
> + if (&tmp->req == _req) {
> + list_del_init(&tmp->queue);
> _req->status = -ECONNRESET;
> + req = tmp;
> retval = 0;
> break;
> }
> }
> spin_unlock(&dum->lock);
>
> - if (retval == 0) {
> + if (req) {
There's no need for this change as we are testing retval, not req here,
unlike the other udc drivers.
So this one I think is correct as-is, or am I mistaken somehow?
thanks,
greg k-h
> On 6. Mar 2022, at 19:00, Greg KH <[email protected]> wrote:
>
> On Sun, Mar 06, 2022 at 06:50:33PM +0100, Jakob Koschel wrote:
>> To move the list iterator variable into the list_for_each_entry_*()
>> macro in the future it should be avoided to use the list iterator
>> variable after the loop body.
>>
>> To *never* use the list iterator variable after the loop it was
>> concluded to use a separate iterator variable [1].
>>
>> This removes the need to check the rc value to determine if the
>> break/goto was hit and can be made more obvious
>> by checking if the variable was set within the list traversal loop.
>>
>> Link: https://lore.kernel.org/all/[email protected]/
>> Signed-off-by: Jakob Koschel <[email protected]>
>> ---
>> drivers/usb/gadget/udc/dummy_hcd.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
>> index a2d956af42a2..f21944707707 100644
>> --- a/drivers/usb/gadget/udc/dummy_hcd.c
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -751,7 +751,7 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> struct dummy *dum;
>> int retval = -EINVAL;
>> unsigned long flags;
>> - struct dummy_request *req = NULL;
>> + struct dummy_request *req = NULL, *tmp;
>>
>> if (!_ep || !_req)
>> return retval;
>> @@ -763,17 +763,18 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>>
>> local_irq_save(flags);
>> spin_lock(&dum->lock);
>> - list_for_each_entry(req, &ep->queue, queue) {
>> - if (&req->req == _req) {
>> - list_del_init(&req->queue);
>> + list_for_each_entry(tmp, &ep->queue, queue) {
>> + if (&tmp->req == _req) {
>> + list_del_init(&tmp->queue);
>> _req->status = -ECONNRESET;
>> + req = tmp;
>> retval = 0;
>> break;
>> }
>> }
>> spin_unlock(&dum->lock);
>>
>> - if (retval == 0) {
>> + if (req) {
>
> There's no need for this change as we are testing retval, not req here,
> unlike the other udc drivers.
>
> So this one I think is correct as-is, or am I mistaken somehow?
The check is correct as-is. I just felt it would be more explicit to
actually check if the pointer that is used within the block is not
NULL than implicitly checking this through retval. There are other
blocks which do like:
list_for_each_entry(pos, head, list) {
if (...) {
rc = -1;
goto fail;
}
}
rc = call_unrelated_function(...);
if (rc == -1)
goto fail;
...
return 0;
fail:
*pos->member;
While this code is obviously broken and then one in this patch works fine,
I feel like it's easier to follow the rule of always checking of pos != NULL.
It might also make it easier for some static analyzers to find potential
NULL pointer dereferences but it probably doesn't matter.
If you prefer keeping retval I'll just do that instead.
>
> thanks,
>
> greg k-h
Jakob