Messages in this thread | | | From | Avi Shchislowski <> | Subject | RE: [RESEND PATCH 1/5] scsi: ufs: Add ufs thermal support | Date | Sun, 29 Mar 2020 07:52:13 +0000 |
| |
> -----Original Message----- > From: Daniel Lezcano <daniel.lezcano@linaro.org> > Sent: Thursday, March 12, 2020 12:38 PM > To: Avi Shchislowski <Avi.Shchislowski@wdc.com>; Avri Altman > <Avri.Altman@wdc.com>; Guenter Roeck <linux@roeck-us.net>; Alim Akhtar > <alim.akhtar@samsung.com>; James E.J. Bottomley <jejb@linux.ibm.com>; > Martin K. Petersen <martin.petersen@oracle.com>; Zhang Rui > <rui.zhang@intel.com>; linux-kernel@vger.kernel.org > Cc: Avi Shchislowski <Avi.Shchislowski@wdc.com> > Subject: Re: [RESEND PATCH 1/5] scsi: ufs: Add ufs thermal support > > On 23/02/2020 10:35, Avi Shchislowski wrote: > > From: Avi Shchislowski <avi.shchislowski@sandisk.com> > > > > Support the new temperature notification attributes introduced in > > UFSv3.0. Add exception event mask, and ufs features attributes. > > > > Signed-off-by: Avi Shchislowski <avi.shchislowski@sandisk.com> > > --- > > drivers/scsi/ufs/Kconfig | 11 ++++ > > drivers/scsi/ufs/Makefile | 1 + > > drivers/scsi/ufs/ufs-thermal.c | 123 > > +++++++++++++++++++++++++++++++++++++++++ > > Why not put the driver under drivers/thermal/ ? > This is not a platform driver, but as our 0 patch indicated, Adding a new feature added by the new ufs spec version (UFS3.0), that allows using the ufs device as a temperature sensor. It require sending a device management commands which are privet to the ufs driver. It also requires responding to a ufs exception events which are privet to the ufs driver. Exposing those externally will potentially allow to brick the storage device
> > drivers/scsi/ufs/ufs-thermal.h | 19 +++++++ > > drivers/scsi/ufs/ufs.h | 11 ++++ > > drivers/scsi/ufs/ufshcd.c | 3 + > > drivers/scsi/ufs/ufshcd.h | 10 ++++ > > 7 files changed, 178 insertions(+) > > create mode 100644 drivers/scsi/ufs/ufs-thermal.c create mode 100644 > > drivers/scsi/ufs/ufs-thermal.h > > > > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig index > > d14c224..bed56ee 100644 > > --- a/drivers/scsi/ufs/Kconfig > > +++ b/drivers/scsi/ufs/Kconfig > > @@ -160,3 +160,14 @@ config SCSI_UFS_BSG > > > > Select this if you need a bsg device node for your UFS controller. > > If unsure, say N. > > + > > +config THERMAL_UFS > > + bool "Thermal UFS" > > + depends on THERMAL && SCSI_UFSHCD > > + help > > + A UFS3.0 feature that allows using the ufs device as a temperature > > + sensor. it provide notification to the host when the UFS device > > + case temperature approaches its pre-defined boundaries. > > + > > + Select Y to enable this feature, otherwise say N. > > + If unsure, say N. > > \ No newline at end of file > > ^^^^ > Done
> > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile > > index 94c6c5d..fd35941 100644 > > --- a/drivers/scsi/ufs/Makefile > > +++ b/drivers/scsi/ufs/Makefile > > @@ -12,3 +12,4 @@ obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += > > ufshcd-pltfrm.o > > obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o > > obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o > > obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o > > +obj-$(CONFIG_THERMAL_UFS) += ufs-thermal.o > > diff --git a/drivers/scsi/ufs/ufs-thermal.c > > b/drivers/scsi/ufs/ufs-thermal.c new file mode 100644 index > > 0000000..469c1ed > > --- /dev/null > > +++ b/drivers/scsi/ufs/ufs-thermal.c > > @@ -0,0 +1,123 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * thermal ufs > > + * > > + * Copyright (C) 2020 Western Digital Corporation */ #include > > +<linux/thermal.h> #include "ufs-thermal.h" > > + > > +enum { > > + UFS_THERM_MAX_TEMP, > > + UFS_THERM_HIGH_TEMP, > > + UFS_THERM_LOW_TEMP, > > + UFS_THERM_MIN_TEMP, > > + > > + /* keep last */ > > + UFS_THERM_MAX_TRIPS > > +}; > > + > > +/** > > + *struct ufs_thermal - thermal zone related data > > + * @tzone: thermal zone device data > > + */ > > +static struct ufs_thermal { > > + struct thermal_zone_device *zone; } thermal; > > + > > +static struct thermal_zone_device_ops ufs_thermal_ops = { > > + .get_temp = NULL, > > + .get_trip_temp = NULL, > > + .get_trip_type = NULL, > > +}; > > Can you merge all the patches related to this driver into a single one? > Yes. Will do.
> > +static int ufs_thermal_enable_ee(struct ufs_hba *hba) { > > + /* later */ > > + return -EINVAL; > > +} > > + > > +static void ufs_thermal_zone_unregister(struct ufs_hba *hba) { > > + if (thermal.zone) { > > + dev_dbg(hba->dev, "Thermal zone device unregister\n"); > > + thermal_zone_device_unregister(thermal.zone); > > + thermal.zone = NULL; > > + } > > +} > > + > > +static int ufs_thermal_register(struct ufs_hba *hba) { > > + int err = 0; > > + char name[THERMAL_NAME_LENGTH] = {}; > > + > > + snprintf(name, THERMAL_NAME_LENGTH, "ufs_storage_%d", > > + hba->host->host_no); > > + > > + thermal.zone = thermal_zone_device_register(name, > UFS_THERM_MAX_TRIPS, > > + 0, hba, &ufs_thermal_ops, NULL, 0, 0); > > + if (IS_ERR(thermal.zone)) { > > + err = PTR_ERR(thermal.zone); > > + dev_err(hba->dev, "Failed to register to thermal zone, err %d\n", > > + err); > > + thermal.zone = NULL; > > + goto out; > > + } > > It is pointless to reassign thermal.zone to NULL. > > As there is no rollback involved here, This can be simplified to: > > if (IS_ERR(thermal.zone)) { > dev_err(hba->dev, "..."); > return PTR_ERR(thermal.zone); > } > > > + > > + /* thermal support is enabled only after successful > > nit: comment format > > /* > * thermal support ... > * ... > */ > Ok. Done
> > + * enablement of thermal exception > > + */ > > + if (ufs_thermal_enable_ee(hba)) { > > + dev_info(hba->dev, "Failed to enable thermal exception\n"); > > + ufs_thermal_zone_unregister(hba); > > + err = -EINVAL; > > + } > > err = ufs_thermal_enable_ee(hba); > if (err) { > ... > return err; > } > > return 0; > Done > > + > > +out: > > + return err; > > +} > > + > > +int ufs_thermal_probe(struct ufs_hba *hba) { > > + u8 ufs_features; > > + u8 *desc_buf = NULL; > > + int err = -EINVAL; > > + > > + if (!ufshcd_thermal_management_enabled(hba)) > > + goto out; > > + > > + desc_buf = kzalloc(hba->desc_size.dev_desc, GFP_KERNEL); > > + if (!desc_buf) { > > + err = -ENOMEM; > > + goto out; > > + } > > + > > + if (ufshcd_read_desc_param(hba, QUERY_DESC_IDN_DEVICE, 0, 0, > desc_buf, > > + hba->desc_size.dev_desc)) > > + goto out; > > + > > + > > + ufs_features = desc_buf[DEVICE_DESC_PARAM_UFS_FEAT] & > > + (UFS_FEATURE_HTEMP | UFS_FEATURE_LTEMP); > > + if (!ufs_features) > > + goto out; > > + > > + err = ufs_thermal_register(hba); > > + if (err) > > + goto out; > > + > > + hba->thermal_features = ufs_features; > > + > > +out: > > + kfree(desc_buf); > > + return err; > > +} > > + > > +void ufs_thermal_remove(struct ufs_hba *hba) { > > + if (!ufshcd_thermal_management_enabled(hba)) > > + return; > > + > > + ufs_thermal_zone_unregister(hba); > > + hba->thermal_features = 0; > > Why is this needed ? > Not needed. Done.
> > +} > > diff --git a/drivers/scsi/ufs/ufs-thermal.h > > b/drivers/scsi/ufs/ufs-thermal.h new file mode 100644 index > > 0000000..7c0fcbe > > --- /dev/null > > +++ b/drivers/scsi/ufs/ufs-thermal.h > > @@ -0,0 +1,19 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2018 Western Digital Corporation */ #ifndef > > +UFS_THERMAL_H #define UFS_THERMAL_H > > + > > +#include "ufshcd.h" > > +#include "ufs.h" > > + > > +#ifdef CONFIG_THERMAL_UFS > > +void ufs_thermal_remove(struct ufs_hba *hba); int > > +ufs_thermal_probe(struct ufs_hba *hba); #else static inline void > > +ufs_thermal_remove(struct ufs_hba *hba) {} static inline int > > +ufs_thermal_probe(struct ufs_hba *hba) {return 0; } #endif /* > > +CONFIG_THERMAL_UFS */ > > + > > +#endif /* UFS_THERMAL_H */ > > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index > > dde2eb0..eb729cc 100644 > > --- a/drivers/scsi/ufs/ufs.h > > +++ b/drivers/scsi/ufs/ufs.h > > @@ -332,6 +332,17 @@ enum { > > UFSHCD_AMP = 3, > > }; > > > > +/* UFS Features - to decode bUFSFeaturesSupport */ enum { > > + UFS_FEATURE_FFU = BIT(0), > > + UFS_FEATURE_PSA = BIT(1), > > + UFS_FEATURE_LIFE = BIT(2), > > + UFS_FEATURE_REFRESH = BIT(3), > > + UFS_FEATURE_HTEMP = BIT(4), > > + UFS_FEATURE_LTEMP = BIT(5), > > + UFS_FEATURE_ETEMP = BIT(6), > > +}; > > + > > #define POWER_DESC_MAX_SIZE 0x62 > > #define POWER_DESC_MAX_ACTV_ICC_LVLS 16 > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index abd0e6b..099d2de 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -47,6 +47,7 @@ > > #include "unipro.h" > > #include "ufs-sysfs.h" > > #include "ufs_bsg.h" > > +#include "ufs-thermal.h" > > > > #define CREATE_TRACE_POINTS > > #include <trace/events/ufs.h> > > @@ -7111,6 +7112,7 @@ static int ufshcd_probe_hba(struct ufs_hba > *hba, > > bool async) > > > > /* Enable Auto-Hibernate if configured */ > > ufshcd_auto_hibern8_enable(hba); > > + ufs_thermal_probe(hba); > > > > out: > > > > @@ -8278,6 +8280,7 @@ int ufshcd_shutdown(struct ufs_hba *hba) > > */ > > void ufshcd_remove(struct ufs_hba *hba) { > > + ufs_thermal_remove(hba); > > ufs_bsg_remove(hba); > > ufs_sysfs_remove_nodes(hba->dev); > > blk_cleanup_queue(hba->tmf_queue); > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > > index 2ae6c7c..28c0063 100644 > > --- a/drivers/scsi/ufs/ufshcd.h > > +++ b/drivers/scsi/ufs/ufshcd.h > > @@ -730,6 +730,11 @@ struct ufs_hba { > > > > struct device bsg_dev; > > struct request_queue *bsg_queue; > > + > > +#define UFSHCD_CAP_THERMAL_MANAGEMENT (1 << 7) > > + > > + u8 thermal_features; > > + > > }; > > > > /* Returns true if clocks can be gated. Otherwise false */ @@ -754,6 > > +759,11 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct > ufs_hba *hba) > > return hba->caps & UFSHCD_CAP_RPM_AUTOSUSPEND; } > > > > +static inline bool ufshcd_thermal_management_enabled(struct ufs_hba > > +*hba) { > > + return hba->caps & UFSHCD_CAP_THERMAL_MANAGEMENT; } > > + > > static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) > > { > > /* DWC UFS Core has the Interrupt aggregation feature but is not > > detectable*/ > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro- > blog/> Blog
| |