To:     Deepak R Varma <drv@mailo.com>
Cc:     James Bottomley <jejb@linux.ibm.com>,
        Khalid Aziz <khalid@gonehiking.org>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Saurabh Singh Sengar <ssengar@microsoft.com>,
        Praveen Kumar <kumarpraveen@linux.microsoft.com>
Subject: Re: [PATCH] scsi: FlashPoint: Replace arithmetic addition by
 bitwise OR
From:   "Martin K. Petersen" <martin.petersen@oracle.com>
Organization: Oracle Corporation
Message-ID: <yq17cwt2s0r.fsf@ca-mkp.ca.oracle.com>
References: <Y+I0HXsHezZRtFOM@ubun2204.myguest.virtualbox.org>
        <9a78cdd254d5d962450242d2e01c3a0f702a63a0.camel@linux.ibm.com>
        <Y+KOeP0OOiem3lR5@ubun2204.myguest.virtualbox.org>
Date:   Tue, 07 Feb 2023 15:19:52 -0500
In-Reply-To: <Y+KOeP0OOiem3lR5@ubun2204.myguest.virtualbox.org> (Deepak
        R. Varma's message of "Tue, 7 Feb 2023 23:16:32 +0530")
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?vWc6DYkYUdiRQ2aaE4eyw533f5wzIdsTtR7KOj/HHrjZlypuLCVImP3mGfhL?=
 =?us-ascii?Q?hNZIYJLl/TDrdI+sGvritHnEunexkTGVsMor9WwpndjrPv64Iwnn5zGXQffN?=
 =?us-ascii?Q?ks7LljAvDSTp8qa44vhxG4TzTzXTKtXbb3L6O0FBauJSxa/sjiyJIAp6JtSI?=
 =?us-ascii?Q?DwZnRG+EUPD0zXt102lVOKjwiPmu2hNZq+35yZLauYt1JDmhNo9wuVAL0ABp?=
 =?us-ascii?Q?ezFYpuRxXVYsFhMIt9sMLSeg7feJy9aXfEfe5d2q739A32cErRzZ1Kvr6Pbf?=
 =?us-ascii?Q?GtzlWE9H0kLn91m7K7Tw+xIWV5JtwdEucwHUtefVKYmiRdtAlMhBLSuQe5UL?=
 =?us-ascii?Q?fBylapYOP4yGoP0NtKQGKlOjbqMWbtMjVN7SDEi/xFVM3gZ7k5YTOQMY+WcY?=
 =?us-ascii?Q?/DlqHmzQUzarphS8xztx0F311W3+CV+K4jqpK/7HrxKOGf+LC6SiI9TFruGn?=
 =?us-ascii?Q?uIHM2sAXr9mG3gup+EyIREuzviZ5D0iYFQRUPGLj3ZZwCjyZ3dKGY1j7L+Ox?=
 =?us-ascii?Q?uw8Zm12mgi67k5U/CSz0NUn0wZOuguuCObyNq5//4REVLB8XxAK/NW+ZrfTs?=
 =?us-ascii?Q?AvnUfXzCVf5DepqOieYp6JDIlw/ecGhTQqs7Ur89RTA28h2PNV7JtDY3+/8Z?=
 =?us-ascii?Q?oTV9f2ZypoZbOTqBB0znLDpnOI1OUrPfp0W6PB66FdmuZFZTwOdQBop5OeHn?=
 =?us-ascii?Q?P292OitkDO9p/nmZdcr8lPNJ7T9wf6KzwR0l1Po86rjDx34/YWxnn02so7ib?=
 =?us-ascii?Q?5Qsn9GB9kwhLcV9kecvlYKJW4S3E9O4jQXrsAwZWFIchbBOKKOo1F7/q8h6e?=
 =?us-ascii?Q?GdgFLXP1fYBQ7gcAOUuhRfJe7ZnwWEJuQKUgwt5P4+Qir3jm0KjW48CAn1Ff?=
 =?us-ascii?Q?XCOf184/CWJm7RVl82Xp+FihS/16k6gfUiPE/wzKNoB45jkK8vkKn6XtQzWz?=
 =?us-ascii?Q?RqSIIPdLGW7AP5VGTpi7rJ4R+6K0cm4HMf9tkUm9HturfCYuYwtiMDkUYY85?=
 =?us-ascii?Q?X2naMlNinVQWRF4NxAuKPscskF8uKF87a7rPh4awkepJu6uBR+S97226X1QZ?=
 =?us-ascii?Q?97gt/GnMGklyPsnsfJebh6MdxodMPLA+gqb8hlIWPieIWN20d/ddfrcAOXT+?=
 =?us-ascii?Q?fnf9btGZVTadt/GwV57HJI45KvZPc5Ns38qz4Xem8sJM1t/3sDFOaAZPfXsY?=
 =?us-ascii?Q?jkfRba1GgGvumJOJL1pL3yO/tU5I2/2GeDeJIkP2VwrLkDCa2cih/MnFd4S7?=
 =?us-ascii?Q?5n04ihb/JQYy5eSMKwKMNSt8u8L3zFCGWeC7xGT0TzIg6LtBailpBkhzydmo?=
 =?us-ascii?Q?T815O4XIdCS6WoUOnB+COpCNxxqPN2JRl+pKKgS8IlO9E2aCmt/NS7kc7p6u?=
 =?us-ascii?Q?jjabmDZN6cRi0D3/HZUtjprEzdg6u/VzDjT+xSynPLaTqpb8DuTtsa6q50FH?=
 =?us-ascii?Q?hlHJRFHX8LHCnjk8jk2jPcOUUo2pmEvRTuzCnSmQRm7fJ/TSodBTdsAsOkAe?=
 =?us-ascii?Q?yVAvokN2Wnn4AYE1+UPuSBOhaKpGFVZ55EiDOHmMPT2wwYnf+sLjrR2/oQGJ?=
 =?us-ascii?Q?XtkmiqqZxZRyAX6B6z/JX85BgYf6UMSmu17EqBbElYoyI+s6MazXxBnjuJhz?=
 =?us-ascii?Q?Cw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?QbARj3dCN76kUr8fUWDUgS+ZyYGMp0j7RrI4NxmM9xOfWXh6dz792g8s89it?=
 =?us-ascii?Q?+Wn0oSwi5ISG02UOeA8r5H2TvxfRlDClXfCOzw01TaoaIQJElAg2rw/Xt00+?=
 =?us-ascii?Q?b9p6pc1gZ0y73GjGb1gcQoovhqyiTwNaRwHRsto90e3WUdZgdEvlcfvppQQO?=
 =?us-ascii?Q?x70NlsADXIf5tREbWfYPn2CBfyXV/Vmwe/hHEC7pCe3tzdSD+goP7YeE2nsv?=
 =?us-ascii?Q?XpUHu6ZAtZORK1D8vW4FhTOIzn7V5LUzBkxiJh4+u9t5NRJjebQ4pcaUpSXD?=
 =?us-ascii?Q?XL++9cazXsCAyQbgJ6dm0lhodHgBxJ3Nj9KtvRua6ndPBAgjXFks3HMflVZy?=
 =?us-ascii?Q?PP/ADv6AXxNCKUdSuouXEyhMWfNmtlCTmERtxYyXmRRpIsMmx6XPgwaqQqAC?=
 =?us-ascii?Q?X1rOrkOuFhmFqQVRpKrRWkjQXI89Obfe2e1P6TfTxDXF66Gtsk195qKYehLz?=
 =?us-ascii?Q?d7cqc1zTtPpAmImBRmJwLjeS5a5/IQYc7Gg7GNw0nAQG8s9zIM6hAd7vapbX?=
 =?us-ascii?Q?mtflndEqvzkFEalCQVUhMZ6YBcMVtRzo+sC2aboj0okFCs7SaRFZI+J/mc+6?=
 =?us-ascii?Q?/GmBZxK3YhOP5i2iOQ93hb8BDSiqieQqNJAmgyFlytomMaDg+kXgUvu98Eru?=
 =?us-ascii?Q?CjxGl+Kxp5RT0KcF6VavBIo8iRdi7TeOAue8UDnJt6LrPzdH9AWLjPNUUnN0?=
 =?us-ascii?Q?CmlP49TwlmxhMQFsc4l9r38+P7V9YJJ1srHgmfCzKmaXJyrHeO+Z1fqxt6Dt?=
 =?us-ascii?Q?WovhQLi9V9rrB3Rz/GUgClvffxWtFarJ6inWNVvNO9qrul8lFQ+gdikOg73Z?=
 =?us-ascii?Q?tabqa0nYuw6BupbYX6M+UHxFW0ukct/HsREuVHwqcfPdQYmDL4pgoToSJt0X?=
 =?us-ascii?Q?WZh/opFnGWMt5Pjp+FUt0xI41uKAsHDpCTCji3VPT5QQcj7DHSRVDQEIWLxG?=
 =?us-ascii?Q?pCjuPjdXAlMtpQbuTQak6w=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9e3cdd4d-74e7-4939-071c-08db0948ad06
X-MS-Exchange-CrossTenant-AuthSource: PH0PR10MB4759.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Feb 2023 20:19:55.0085
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TQ96U5iHNaM/oRh4jV7DbaeIw3ty6YCs5r+jnvAuC+F2xSUMVL3BQkKwv7WnA74P65fRFa4smso6/wq2OYnXrAUU/Ko3Vueq1rCFwYs2JSQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR10MB5560
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-02-07_11,2023-02-06_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0 spamscore=0
 malwarescore=0 suspectscore=0 mlxscore=0 mlxlogscore=742 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2302070176
X-Proofpoint-GUID: Pxr_DdiJiP1yXKASOffDMI6KhWBUTQD0
X-Proofpoint-ORIG-GUID: Pxr_DdiJiP1yXKASOffDMI6KhWBUTQD0
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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!

-- 
Martin K. Petersen	Oracle Linux Engineering