linux-kernel - Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise OR

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] [day] [month] [year] [list]

Date:   Thu, 9 Feb 2023 13:02:26 +0530
From:   Deepak R Varma <drv@...lo.com>
To:     "Martin K. Petersen" <martin.petersen@...cle.com>
Cc:     James Bottomley <jejb@...ux.ibm.com>,
        Khalid Aziz <khalid@...ehiking.org>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Saurabh Singh Sengar <ssengar@...rosoft.com>,
        Praveen Kumar <kumarpraveen@...ux.microsoft.com>
Subject: Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by bitwise
 OR

On Tue, Feb 07, 2023 at 03:19:52PM -0500, Martin K. Petersen wrote:
> 
> Hi Deepak!
> 
> > James, there are a few other patch submissions for the scsi subsystem
> > that I submitted in last few weeks. I sent couple of reminder request
> > for comments on those submission, but still waiting. Could you please
> > also review those and share your feedback?
> 
> I only merge cleanup patches if the relevant driver maintainer reviews
> and acks them. And there needs to be sufficient justification, of
> course.
> 
> As an example I will observe that your sysfs patches say:
> 
> "According to Documentation/filesystems/sysfs.rst, the show() callback
> function of kobject attributes should strictly use sysfs_emit() instead
> of sprintf() family functions."
> 
> That's a "should", not a "shall". IOW, a recommendation. Also, there is
> no "strictly" requirement to be found in sysfs.rst. So the patch
> justification is lacking.
> 
> We definitely use sysfs_emit() for new code. But it is not clear that
> there is any value in changing old code predating sysfs_emit() to comply
> with a mere recommendation. It's just additional work and comes with a
> substantial risk of introducing regressions since virtually no cleanup
> patches have actually been tested on the relevant hardware.
> 
> Nobody has access to all the devices required to validate the many, many
> drivers we have in SCSI. So unless the driver maintainer (who presumably
> has hardware) is willing to test, it is impossible for me to qualify
> whether a patch introduces a regression.
> 
> One option is demonstrating that the patch does not change the generated
> object code as James suggested. But even if the binary is unchanged,
> cleanups often involve stylistic changes or switching to a more "modern"
> approach. And for those changes I still want the driver maintainer to
> ack. It's their driver.
> 
> To pick another example from your posted patches, it not immediately
> obvious that using min() is superior to an if-else statement. Sometimes
> it is clearer to express things as a conditional even though it takes up
> more vertical space (or maybe because it does?). In any case that's a
> personal preference and the driver maintainer's choice.
> 
> Hope that helps!

Hello James and Martin,
I appreciate your comments and the detailed response. Thank you very much. I
understand the proposals are mostly clean up and not a significant benefit to
the code in terms of bug-fix or improvement. I will leave the patches at the
current status if any driver maintainer wants to take those forward.

Thank you again and apologies for any inconvenience.
./drv

> 
> -- 
> Martin K. Petersen	Oracle Linux Engineering