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

[Deepak R Varma]

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