| 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
Hi Martin, This patch series includes the following changes: - Separation of UFS core and UFS driver source code into separate directories. - Split the ufshcd.h header file into two header files - one file that defines the interface with UFS drivers and another file with definitions only used in the core. - Multiple source code cleanup patches. - A few patches with minor functional changes. Please consider these changes for kernel v5.19. Thank you, Bart. Changes compared to v1: * Added a new patch with a source code comment spelling fix. * Included a HPB change in patch "Simplify statements that return a boolean". * Removed a superfluous test from patch "Remove ufshcd_lrb.sense_buffer". * Dropped patch "Remove the LUN quiescing code from ufshcd_wl_shutdown()". * Fixed indentation in patch "Make the config_scaling_param calls type safe". * Improved the description of patch "Remove locking from around single register writes". * Modified patch "Minimize #include directives" such that the current order of #include directives is preserved. * Fixed support for CONFIG_SCSI_UFS_HWMON=n in patch "Split the ufshcd.h header file". * In patch "Split the drivers/scsi/ufs directory", moved the UFS source files from drivers/scsi/ufs/ into drivers/ufs/. Bart Van Assche (29): scsi: ufs: Fix a spelling error in a source code comment scsi: ufs: Declare ufshcd_wait_for_register() static scsi: ufs: Remove superfluous boolean conversions scsi: ufs: Simplify statements that return a boolean scsi: ufs: Remove ufshcd_lrb.sense_bufflen scsi: ufs: Remove ufshcd_lrb.sense_buffer scsi: ufs: Use get_unaligned_be16() instead of be16_to_cpup() scsi: ufs: Remove the UFS_FIX() and END_FIX() macros scsi: ufs: Rename struct ufs_dev_fix into ufs_dev_quirk scsi: ufs: Declare the quirks array const scsi: ufs: Invert the return value of ufshcd_is_hba_active() scsi: ufs: Remove unused constants and code scsi: ufs: Switch to aggregate initialization scsi: ufs: Make the config_scaling_param calls type safe scsi: ufs: Remove the driver version scsi: ufs: Rename sdev_ufs_device into ufs_device_wlun scsi: ufs: Use an SPDX license identifier in the Kconfig file scsi: ufs: Remove paths from source code comments scsi: ufs: Remove the TRUE and FALSE definitions scsi: ufs: Remove locking from around single register writes scsi: ufs: Introduce ufshcd_clkgate_delay_set() scsi: ufs: qcom: Fix ufs_qcom_resume() scsi: ufs: Remove unnecessary ufshcd-crypto.h include directives scsi: ufs: Fix kernel-doc syntax in ufshcd.h scsi: ufs: Minimize #include directives scsi: ufs: Split the ufshcd.h header file scsi: ufs: Move the struct ufs_ref_clk definition scsi: ufs: Move the ufs_is_valid_unit_desc_lun() definition scsi: ufs: Split the drivers/scsi/ufs directory MAINTAINERS | 9 +- drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/scsi/Kconfig | 1 - drivers/scsi/Makefile | 1 - drivers/scsi/ufs/Kconfig | 211 ---------- drivers/ufs/Kconfig | 30 ++ drivers/ufs/Makefile | 5 + drivers/ufs/core/Kconfig | 60 +++ drivers/ufs/core/Makefile | 10 + drivers/{scsi/ufs => ufs/core}/ufs-debugfs.c | 3 +- drivers/{scsi/ufs => ufs/core}/ufs-debugfs.h | 0 .../ufs => ufs/core}/ufs-fault-injection.c | 0 .../ufs => ufs/core}/ufs-fault-injection.h | 0 drivers/{scsi/ufs => ufs/core}/ufs-hwmon.c | 3 +- drivers/{scsi/ufs => ufs/core}/ufs-sysfs.c | 3 +- drivers/{scsi/ufs => ufs/core}/ufs-sysfs.h | 3 +- drivers/{scsi/ufs => ufs/core}/ufs_bsg.c | 6 + drivers/{scsi/ufs => ufs/core}/ufs_bsg.h | 7 +- .../{scsi/ufs => ufs/core}/ufshcd-crypto.c | 2 +- .../{scsi/ufs => ufs/core}/ufshcd-crypto.h | 7 +- drivers/ufs/core/ufshcd-priv.h | 298 ++++++++++++++ drivers/{scsi/ufs => ufs/core}/ufshcd.c | 236 +++++------ drivers/{scsi/ufs => ufs/core}/ufshpb.c | 16 +- drivers/{scsi/ufs => ufs/core}/ufshpb.h | 0 drivers/ufs/host/Kconfig | 114 ++++++ drivers/{scsi/ufs => ufs/host}/Makefile | 12 - drivers/{scsi/ufs => ufs/host}/cdns-pltfrm.c | 2 +- .../{scsi/ufs => ufs/host}/tc-dwc-g210-pci.c | 3 +- .../ufs => ufs/host}/tc-dwc-g210-pltfrm.c | 1 + drivers/{scsi/ufs => ufs/host}/tc-dwc-g210.c | 6 +- drivers/{scsi/ufs => ufs/host}/tc-dwc-g210.h | 2 + drivers/{scsi/ufs => ufs/host}/ti-j721e-ufs.c | 0 drivers/{scsi/ufs => ufs/host}/ufs-exynos.c | 11 +- drivers/{scsi/ufs => ufs/host}/ufs-exynos.h | 8 +- drivers/{scsi/ufs => ufs/host}/ufs-hisi.c | 10 +- drivers/{scsi/ufs => ufs/host}/ufs-hisi.h | 0 .../ufs => ufs/host}/ufs-mediatek-trace.h | 2 +- drivers/{scsi/ufs => ufs/host}/ufs-mediatek.c | 37 +- drivers/{scsi/ufs => ufs/host}/ufs-mediatek.h | 0 drivers/{scsi/ufs => ufs/host}/ufs-qcom-ice.c | 2 +- drivers/{scsi/ufs => ufs/host}/ufs-qcom.c | 36 +- drivers/{scsi/ufs => ufs/host}/ufs-qcom.h | 6 +- drivers/{scsi/ufs => ufs/host}/ufshcd-dwc.c | 6 +- drivers/{scsi/ufs => ufs/host}/ufshcd-dwc.h | 2 + drivers/{scsi/ufs => ufs/host}/ufshcd-pci.c | 6 +- .../{scsi/ufs => ufs/host}/ufshcd-pltfrm.c | 32 +- .../{scsi/ufs => ufs/host}/ufshcd-pltfrm.h | 2 +- drivers/{scsi/ufs => ufs/host}/ufshci-dwc.h | 0 {drivers/scsi/ufs => include/scsi}/ufs.h | 35 -- .../scsi/ufs => include/scsi}/ufs_quirks.h | 15 +- {drivers/scsi/ufs => include/scsi}/ufshcd.h | 376 ++++-------------- {drivers/scsi/ufs => include/scsi}/ufshci.h | 2 + {drivers/scsi/ufs => include/scsi}/unipro.h | 18 +- 54 files changed, 840 insertions(+), 820 deletions(-) delete mode 100644 drivers/scsi/ufs/Kconfig create mode 100644 drivers/ufs/Kconfig create mode 100644 drivers/ufs/Makefile create mode 100644 drivers/ufs/core/Kconfig create mode 100644 drivers/ufs/core/Makefile rename drivers/{scsi/ufs => ufs/core}/ufs-debugfs.c (99%) rename drivers/{scsi/ufs => ufs/core}/ufs-debugfs.h (100%) rename drivers/{scsi/ufs => ufs/core}/ufs-fault-injection.c (100%) rename drivers/{scsi/ufs => ufs/core}/ufs-fault-injection.h (100%) rename drivers/{scsi/ufs => ufs/core}/ufs-hwmon.c (98%) rename drivers/{scsi/ufs => ufs/core}/ufs-sysfs.c (99%) rename drivers/{scsi/ufs => ufs/core}/ufs-sysfs.h (95%) rename drivers/{scsi/ufs => ufs/core}/ufs_bsg.c (97%) rename drivers/{scsi/ufs => ufs/core}/ufs_bsg.h (77%) rename drivers/{scsi/ufs => ufs/core}/ufshcd-crypto.c (99%) rename drivers/{scsi/ufs => ufs/core}/ufshcd-crypto.h (94%) create mode 100644 drivers/ufs/core/ufshcd-priv.h rename drivers/{scsi/ufs => ufs/core}/ufshcd.c (98%) rename drivers/{scsi/ufs => ufs/core}/ufshpb.c (99%) rename drivers/{scsi/ufs => ufs/core}/ufshpb.h (100%) create mode 100644 drivers/ufs/host/Kconfig rename drivers/{scsi/ufs => ufs/host}/Makefile (56%) rename drivers/{scsi/ufs => ufs/host}/cdns-pltfrm.c (99%) rename drivers/{scsi/ufs => ufs/host}/tc-dwc-g210-pci.c (98%) rename drivers/{scsi/ufs => ufs/host}/tc-dwc-g210-pltfrm.c (98%) rename drivers/{scsi/ufs => ufs/host}/tc-dwc-g210.c (99%) rename drivers/{scsi/ufs => ufs/host}/tc-dwc-g210.h (95%) rename drivers/{scsi/ufs => ufs/host}/ti-j721e-ufs.c (100%) rename drivers/{scsi/ufs => ufs/host}/ufs-exynos.c (99%) rename drivers/{scsi/ufs => ufs/host}/ufs-exynos.h (97%) rename drivers/{scsi/ufs => ufs/host}/ufs-hisi.c (99%) rename drivers/{scsi/ufs => ufs/host}/ufs-hisi.h (100%) rename drivers/{scsi/ufs => ufs/host}/ufs-mediatek-trace.h (93%) rename drivers/{scsi/ufs => ufs/host}/ufs-mediatek.c (97%) rename drivers/{scsi/ufs => ufs/host}/ufs-mediatek.h (100%) rename drivers/{scsi/ufs => ufs/host}/ufs-qcom-ice.c (99%) rename drivers/{scsi/ufs => ufs/host}/ufs-qcom.c (98%) rename drivers/{scsi/ufs => ufs/host}/ufs-qcom.h (98%) rename drivers/{scsi/ufs => ufs/host}/ufshcd-dwc.c (98%) rename drivers/{scsi/ufs => ufs/host}/ufshcd-dwc.h (95%) rename drivers/{scsi/ufs => ufs/host}/ufshcd-pci.c (99%) rename drivers/{scsi/ufs => ufs/host}/ufshcd-pltfrm.c (94%) rename drivers/{scsi/ufs => ufs/host}/ufshcd-pltfrm.h (98%) rename drivers/{scsi/ufs => ufs/host}/ufshci-dwc.h (100%) rename {drivers/scsi/ufs => include/scsi}/ufs.h (93%) rename {drivers/scsi/ufs => include/scsi}/ufs_quirks.h (94%) rename {drivers/scsi/ufs => include/scsi}/ufshcd.h (81%) rename {drivers/scsi/ufs => include/scsi}/ufshci.h (99%) rename {drivers/scsi/ufs => include/scsi}/unipro.h (98%)