Message ID | 20220412181853.3715080-1-bvanassche@acm.org |
---|---|
Headers | show |
Series | UFS patches for kernel v5.19 | expand |
On Tue, Apr 12, 2022 at 11:18:31AM -0700, Bart Van Assche wrote: > Use get_unaligned_be16(...) instead of the equivalent but harder to read > be16_to_cpup((__be16 *)...). > > Reviewed-by: Bean Huo <beanhuo@micron.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index d4ef31e1a409..3ec26c9eb1be 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -7334,7 +7334,7 @@ static u32 ufshcd_get_max_icc_level(int sup_curr_uA, u32 start_scan, char *buff) > u16 unit; > > for (i = start_scan; i >= 0; i--) { > - data = be16_to_cpup((__be16 *)&buff[2 * i]); > + data = get_unaligned_be16(&buff[2 * i]); This is not "equivalent". get_unaligned_be16() works on unaligned values whereas be16_to_cpup() assumes a naturally aligned value. This patch might still be the right thing to do, but the explanation is not correct. - Eric
On 4/12/22 14:26, Eric Biggers wrote: > On Tue, Apr 12, 2022 at 11:18:31AM -0700, Bart Van Assche wrote: >> Use get_unaligned_be16(...) instead of the equivalent but harder to read >> be16_to_cpup((__be16 *)...). >> >> Reviewed-by: Bean Huo <beanhuo@micron.com> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index d4ef31e1a409..3ec26c9eb1be 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -7334,7 +7334,7 @@ static u32 ufshcd_get_max_icc_level(int sup_curr_uA, u32 start_scan, char *buff) >> u16 unit; >> >> for (i = start_scan; i >= 0; i--) { >> - data = be16_to_cpup((__be16 *)&buff[2 * i]); >> + data = get_unaligned_be16(&buff[2 * i]); > > This is not "equivalent". get_unaligned_be16() works on unaligned values > whereas be16_to_cpup() assumes a naturally aligned value. This patch might > still be the right thing to do, but the explanation is not correct. This is what I found in an English dictionary for the word equivalent: "corresponding or virtually identical especially in effect or function". The effect of this patch is that the same value will be stored in 'data' as without this patch. Additionally, no runtime error will be generated with the patch applied if the original code did not trigger a runtime exception. I think that how I used the word "equivalent" is consistent with the explanation I found in an English dictionary. Thanks, Bart.
On 4/12/22 19:33, Keoseong Park wrote: > Hi Bart, > >> Convert "if (expr) return true; else return false;" into "return expr;" >> if either 'expr' is a boolean expression or the return type of the >> function is 'bool'. > > How about adding ufshcd_is_pwr_mode_restore_needed()? Hi Keoseong, I'd like to keep that function as-is because it has three return statements instead of two. Thanks, Bart.
> Change one occurrence of "adpater" into "adapter". > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com>
>On 4/12/22 19:33, Keoseong Park wrote: >> Hi Bart, >> >>> Convert "if (expr) return true; else return false;" into "return expr;" >>> if either 'expr' is a boolean expression or the return type of the >>> function is 'bool'. >> >> How about adding ufshcd_is_pwr_mode_restore_needed()? > >Hi Keoseong, > >I'd like to keep that function as-is because it has three return >statements instead of two. > >Thanks, > >Bart. I get it. Reviewed-by: Keoseong Park <keosung.park@samsung.com> Best Regards, Keoseong Park