linux-kernel - Re: [PATCH v5 15/19] staging: r8188eu: hal: Clean up usbctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210915135301.GF2088@kadam>
Date:   Wed, 15 Sep 2021 16:53:01 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Philip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Pavel Skripkin <paskripkin@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        David Laight <david.Laight@...lab.com>
Subject: Re: [PATCH v5 15/19] staging: r8188eu: hal: Clean up
 usbctrl_vendorreq()

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