Re: [PATCH v5 15/19] staging: r8188eu: hal: Clean up usbctrl_vendorreq()

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Sep 15, 2021 at 02:41:45PM +0200, Fabio M. De Francesco wrote:
> Clean up usbctrl_vendoreq () in usb_ops_linux.c. Eventually this function
> will be deleted but some of the code will be reused later. This cleanup
> makes code reuse easier.
>

Thanks for removing the URL.  This commit message is no longer bad to
the point where it has to be redone but it's still not great.

I explicitly told you to leave the irrelevant information out.  I'm
trying to help you and it's frustrating that you're not listening.  I
wish that you had just copy and pasted the commit message which I sent.

This relates the discussion we had about reviewing patches one at a time
in the order they arrive.  Every patch should be self contained.  It
should not refer to the past except in the case of explaining the Fixes
tag and it should not refer to the future except in the case where it
needs to excuse adding unused infrastructure.  Reviewing is stateless.
We don't want to know about your plans.

On the other hand, the commit message doesn't list the changes the
commit makes as part of the clean up process.  That would have been
helpful information for me as a reviewer.

*Sigh*  Whatever...  I would have allowed this commit message but there
is a bug in the code.

> +				memcpy(data, io_buf,  len);
> +		} else {
> +			/* errors */
>  			if (status < 0) {
> -				if (status == (-ESHUTDOWN) || status == -ENODEV) {
> +				if (status == (-ESHUTDOWN || -ENODEV)) {

This is a bug so you'll have to redo the patch.

regards,
dan carpenter