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
