| Message ID | 1517501746-19075-2-git-send-email-stanislav.nijnikov@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Feb 01, 2018 at 06:15:37PM +0200, Stanislav Nijnikov wrote: > This patch introduces attribute group to show existing sysfs entries. > > Signed-off-by: Stanislav Nijnikov <stanislav.nijnikov@wdc.com> > --- > drivers/scsi/ufs/Makefile | 3 +- > drivers/scsi/ufs/ufs-sysfs.c | 164 +++++++++++++++++++++++++++++++++++++++++++ > drivers/scsi/ufs/ufs-sysfs.h | 22 ++++++ > drivers/scsi/ufs/ufshcd.c | 156 ++-------------------------------------- > drivers/scsi/ufs/ufshcd.h | 2 + > 5 files changed, 194 insertions(+), 153 deletions(-) > create mode 100644 drivers/scsi/ufs/ufs-sysfs.c > create mode 100644 drivers/scsi/ufs/ufs-sysfs.h > > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile > index 9310c6c..918f579 100644 > --- a/drivers/scsi/ufs/Makefile > +++ b/drivers/scsi/ufs/Makefile > @@ -3,6 +3,7 @@ > obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o tc-dwc-g210.o > obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-dwc-g210.o > obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o > -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o > +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o > +ufshcd-core-objs := ufshcd.o ufs-sysfs.o > obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o > obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c > new file mode 100644 > index 0000000..cc68a90 > --- /dev/null > +++ b/drivers/scsi/ufs/ufs-sysfs.c > @@ -0,0 +1,164 @@ > +//SPDX-License-Identifier: GPL-2.0-only > +//Copyright (C) 2018 Western Digital Corporation > +//This program is free software; you can redistribute it and/or modify it > +//under the terms of the GNU General Public License as published by the > +//Free Software Foundation; version 2. > +// > +//This program is distributed in the hope that it will be useful, but > +//WITHOUT ANY WARRANTY; without even the implied warranty of > +//MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +//General Public License for more details. No need to put the whole "This program" crud in the file here if you have the SPDX line already. We are going through the kernel tree and removing all of the 700+ different ways we have this boilerplate code in the tree, please do not add new ones. Also, please put a ' ' after "//", it just looks ugly like this, don't you think so? Please fix this for all of the files you add in this patch series. > +#include <linux/err.h> > +#include <linux/string.h> > + > +#include "ufs-sysfs.h" > + > +static const char *ufschd_uic_link_state_to_string( > + enum uic_link_state state) > +{ > + switch (state) { > + case UIC_LINK_OFF_STATE: return "OFF"; > + case UIC_LINK_ACTIVE_STATE: return "ACTIVE"; > + case UIC_LINK_HIBERN8_STATE: return "HIBERN8"; > + default: return "UNKNOWN"; > + } > +} > + > +static const char *ufschd_ufs_dev_pwr_mode_to_string( > + enum ufs_dev_pwr_mode state) > +{ > + switch (state) { > + case UFS_ACTIVE_PWR_MODE: return "ACTIVE"; > + case UFS_SLEEP_PWR_MODE: return "SLEEP"; > + case UFS_POWERDOWN_PWR_MODE: return "POWERDOWN"; > + default: return "UNKNOWN"; > + } > +} > + > +static inline ssize_t ufs_sysfs_pm_lvl_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count, > + bool rpm) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned long flags, value; > + > + if (kstrtoul(buf, 0, &value)) > + return -EINVAL; > + > + if (value >= UFS_PM_LVL_MAX) > + return -EINVAL; > + > + spin_lock_irqsave(hba->host->host_lock, flags); > + if (rpm) > + hba->rpm_lvl = value; > + else > + hba->spm_lvl = value; > + spin_unlock_irqrestore(hba->host->host_lock, flags); > + return count; > +} > + > +static ssize_t rpm_lvl_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + int curr_len; > + u8 lvl; > + > + curr_len = snprintf(buf, PAGE_SIZE, > + "\nCurrent Runtime PM level [%d] => dev_state [%s] link_state [%s]\n", > + hba->rpm_lvl, > + ufschd_ufs_dev_pwr_mode_to_string( > + ufs_pm_lvl_states[hba->rpm_lvl].dev_state), > + ufschd_uic_link_state_to_string( > + ufs_pm_lvl_states[hba->rpm_lvl].link_state)); > + > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > + "\nAll available Runtime PM levels info:\n"); > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > + "\tRuntime PM level [%d] => dev_state [%s] link_state [%s]\n", > + lvl, > + ufschd_ufs_dev_pwr_mode_to_string( > + ufs_pm_lvl_states[lvl].dev_state), > + ufschd_uic_link_state_to_string( > + ufs_pm_lvl_states[lvl].link_state)); > + sysfs if "one value per file", not "random text that someone has to parse per file" please. Huge hint, if you ever care about checking the size of the sysfs buffer you are writing into, you are doing something really really wrong. > + return curr_len; > +} > + > +static ssize_t rpm_lvl_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ > + return ufs_sysfs_pm_lvl_store(dev, attr, buf, count, true); > +} > + > +static ssize_t spm_lvl_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + int curr_len; > + u8 lvl; > + > + curr_len = snprintf(buf, PAGE_SIZE, > + "\nCurrent System PM level [%d] => dev_state [%s] link_state [%s]\n", > + hba->spm_lvl, > + ufschd_ufs_dev_pwr_mode_to_string( > + ufs_pm_lvl_states[hba->spm_lvl].dev_state), > + ufschd_uic_link_state_to_string( > + ufs_pm_lvl_states[hba->spm_lvl].link_state)); > + > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > + "\nAll available System PM levels info:\n"); > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > + "\tSystem PM level [%d] => dev_state [%s] link_state [%s]\n", > + lvl, > + ufschd_ufs_dev_pwr_mode_to_string( > + ufs_pm_lvl_states[lvl].dev_state), > + ufschd_uic_link_state_to_string( > + ufs_pm_lvl_states[lvl].link_state)); > + Same here, this output is not ok. > + return curr_len; > +} > + > +static ssize_t spm_lvl_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ > + return ufs_sysfs_pm_lvl_store(dev, attr, buf, count, false); > +} > + > +static DEVICE_ATTR_RW(rpm_lvl); > +static DEVICE_ATTR_RW(spm_lvl); > + > +static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > + &dev_attr_rpm_lvl.attr, > + &dev_attr_spm_lvl.attr, > + NULL > +}; > + > +static const struct attribute_group ufs_sysfs_default_group = { > + .attrs = ufs_sysfs_ufshcd_attrs, > +}; > + > +static const struct attribute_group *ufs_sysfs_groups[] = { > + &ufs_sysfs_default_group, > + NULL, > +}; ATTRIBUTE_GROUPS() macro? > +void ufs_sysfs_add_nodes(struct device *dev) > +{ > + int ret; > + > + ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups); > + if (ret) > + dev_err(dev, > + "%s: sysfs groups creation failed (err = %d)\n", > + __func__, ret); Why not return 'ret' so you can do something with it? thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Thursday, February 1, 2018 6:59 PM > To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com> > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > jaegeuk@kernel.org; Alex Lemberg <Alex.Lemberg@wdc.com> > Subject: Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs > entries. > > On Thu, Feb 01, 2018 at 06:15:37PM +0200, Stanislav Nijnikov wrote: > > This patch introduces attribute group to show existing sysfs entries. > > > > Signed-off-by: Stanislav Nijnikov <stanislav.nijnikov@wdc.com> > > --- > > drivers/scsi/ufs/Makefile | 3 +- > > drivers/scsi/ufs/ufs-sysfs.c | 164 > > +++++++++++++++++++++++++++++++++++++++++++ > > drivers/scsi/ufs/ufs-sysfs.h | 22 ++++++ > > drivers/scsi/ufs/ufshcd.c | 156 ++-------------------------------------- > > drivers/scsi/ufs/ufshcd.h | 2 + > > 5 files changed, 194 insertions(+), 153 deletions(-) create mode > > 100644 drivers/scsi/ufs/ufs-sysfs.c create mode 100644 > > drivers/scsi/ufs/ufs-sysfs.h > > > > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile > > index 9310c6c..918f579 100644 > > --- a/drivers/scsi/ufs/Makefile > > +++ b/drivers/scsi/ufs/Makefile > > @@ -3,6 +3,7 @@ > > obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd- > dwc.o > > tc-dwc-g210.o > > obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o > > ufshcd-dwc.o tc-dwc-g210.o > > obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o > > -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o > > +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o ufshcd-core-objs := > > +ufshcd.o ufs-sysfs.o > > obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o > > obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o diff --git > > a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c new file > > mode 100644 index 0000000..cc68a90 > > --- /dev/null > > +++ b/drivers/scsi/ufs/ufs-sysfs.c > > @@ -0,0 +1,164 @@ > > +//SPDX-License-Identifier: GPL-2.0-only //Copyright (C) 2018 Western > > +Digital Corporation //This program is free software; you can > > +redistribute it and/or modify it //under the terms of the GNU General > > +Public License as published by the //Free Software Foundation; > > +version 2. > > +// > > +//This program is distributed in the hope that it will be useful, but > > +//WITHOUT ANY WARRANTY; without even the implied warranty of > > +//MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > GNU > > +//General Public License for more details. > > No need to put the whole "This program" crud in the file here if you have the > SPDX line already. We are going through the kernel tree and removing all of > the 700+ different ways we have this boilerplate code in the tree, please do > not add new ones. > > Also, please put a ' ' after "//", it just looks ugly like this, don't you think so? > > Please fix this for all of the files you add in this patch series. > > > +#include <linux/err.h> > > +#include <linux/string.h> > > + > > +#include "ufs-sysfs.h" > > + > > +static const char *ufschd_uic_link_state_to_string( > > + enum uic_link_state state) > > +{ > > + switch (state) { > > + case UIC_LINK_OFF_STATE: return "OFF"; > > + case UIC_LINK_ACTIVE_STATE: return "ACTIVE"; > > + case UIC_LINK_HIBERN8_STATE: return "HIBERN8"; > > + default: return "UNKNOWN"; > > + } > > +} > > + > > +static const char *ufschd_ufs_dev_pwr_mode_to_string( > > + enum ufs_dev_pwr_mode state) > > +{ > > + switch (state) { > > + case UFS_ACTIVE_PWR_MODE: return "ACTIVE"; > > + case UFS_SLEEP_PWR_MODE: return "SLEEP"; > > + case UFS_POWERDOWN_PWR_MODE: return "POWERDOWN"; > > + default: return "UNKNOWN"; > > + } > > +} > > + > > +static inline ssize_t ufs_sysfs_pm_lvl_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count, > > + bool rpm) > > +{ > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > + unsigned long flags, value; > > + > > + if (kstrtoul(buf, 0, &value)) > > + return -EINVAL; > > + > > + if (value >= UFS_PM_LVL_MAX) > > + return -EINVAL; > > + > > + spin_lock_irqsave(hba->host->host_lock, flags); > > + if (rpm) > > + hba->rpm_lvl = value; > > + else > > + hba->spm_lvl = value; > > + spin_unlock_irqrestore(hba->host->host_lock, flags); > > + return count; > > +} > > + > > +static ssize_t rpm_lvl_show(struct device *dev, > > + struct device_attribute *attr, char *buf) { > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > + int curr_len; > > + u8 lvl; > > + > > + curr_len = snprintf(buf, PAGE_SIZE, > > + "\nCurrent Runtime PM level [%d] => dev_state > [%s] link_state [%s]\n", > > + hba->rpm_lvl, > > + ufschd_ufs_dev_pwr_mode_to_string( > > + ufs_pm_lvl_states[hba- > >rpm_lvl].dev_state), > > + ufschd_uic_link_state_to_string( > > + ufs_pm_lvl_states[hba- > >rpm_lvl].link_state)); > > + > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > + "\nAll available Runtime PM levels info:\n"); > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > + "\tRuntime PM level [%d] => dev_state > [%s] link_state [%s]\n", > > + lvl, > > + ufschd_ufs_dev_pwr_mode_to_string( > > + ufs_pm_lvl_states[lvl].dev_state), > > + ufschd_uic_link_state_to_string( > > + ufs_pm_lvl_states[lvl].link_state)); > > + > > sysfs if "one value per file", not "random text that someone has to parse per > file" please. > > Huge hint, if you ever care about checking the size of the sysfs buffer you are > writing into, you are doing something really really wrong. > Hi Greg It's the existing code, added by: commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12 Author: subhashj@codeaurora.org <subhashj@codeaurora.org> Date: Thu Dec 22 18:41:00 2016 -0800 scsi: ufs: provide sysfs attribute to select the PM level This patch provides the sysfs attribute to choose the power management level for UFS runtime and system suspend. Reviewed-by: Sujit Reddy Thumma <sthumma@codeaurora.org> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> I just moved it to an another file and changed the sysfs entries creation by Jaegeuk Kim' request. At the moment the entry shows the PM level, the device state, the link state and all possible PM levels. Do you want me to change it? > > > + return curr_len; > > +} > > + > > +static ssize_t rpm_lvl_store(struct device *dev, > > + struct device_attribute *attr, const char *buf, size_t count) { > > + return ufs_sysfs_pm_lvl_store(dev, attr, buf, count, true); } > > + > > +static ssize_t spm_lvl_show(struct device *dev, > > + struct device_attribute *attr, char *buf) { > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > + int curr_len; > > + u8 lvl; > > + > > + curr_len = snprintf(buf, PAGE_SIZE, > > + "\nCurrent System PM level [%d] => dev_state > [%s] link_state [%s]\n", > > + hba->spm_lvl, > > + ufschd_ufs_dev_pwr_mode_to_string( > > + ufs_pm_lvl_states[hba- > >spm_lvl].dev_state), > > + ufschd_uic_link_state_to_string( > > + ufs_pm_lvl_states[hba- > >spm_lvl].link_state)); > > + > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > + "\nAll available System PM levels info:\n"); > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > + "\tSystem PM level [%d] => dev_state > [%s] link_state [%s]\n", > > + lvl, > > + ufschd_ufs_dev_pwr_mode_to_string( > > + ufs_pm_lvl_states[lvl].dev_state), > > + ufschd_uic_link_state_to_string( > > + ufs_pm_lvl_states[lvl].link_state)); > > + > > Same here, this output is not ok. > Same here. > > + return curr_len; > > +} > > + > > +static ssize_t spm_lvl_store(struct device *dev, > > + struct device_attribute *attr, const char *buf, size_t count) { > > + return ufs_sysfs_pm_lvl_store(dev, attr, buf, count, false); } > > + > > +static DEVICE_ATTR_RW(rpm_lvl); > > +static DEVICE_ATTR_RW(spm_lvl); > > + > > +static struct attribute *ufs_sysfs_ufshcd_attrs[] = { > > + &dev_attr_rpm_lvl.attr, > > + &dev_attr_spm_lvl.attr, > > + NULL > > +}; > > + > > +static const struct attribute_group ufs_sysfs_default_group = { > > + .attrs = ufs_sysfs_ufshcd_attrs, > > +}; > > + > > +static const struct attribute_group *ufs_sysfs_groups[] = { > > + &ufs_sysfs_default_group, > > + NULL, > > +}; > > ATTRIBUTE_GROUPS() macro? > > > +void ufs_sysfs_add_nodes(struct device *dev) { > > + int ret; > > + > > + ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups); > > + if (ret) > > + dev_err(dev, > > + "%s: sysfs groups creation failed (err = %d)\n", > > + __func__, ret); > > Why not return 'ret' so you can do something with it? The existing in the driver functions that create the sysfs entries do nothing on failure, just error message. So I just keep the same logic. > > thanks, > > greg k-h Thank you. Stanislav
On Sun, Feb 04, 2018 at 12:29:06PM +0000, Stanislav Nijnikov wrote: > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > > + "\nAll available Runtime PM levels info:\n"); > > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > > + "\tRuntime PM level [%d] => dev_state > > [%s] link_state [%s]\n", > > > + lvl, > > > + ufschd_ufs_dev_pwr_mode_to_string( > > > + ufs_pm_lvl_states[lvl].dev_state), > > > + ufschd_uic_link_state_to_string( > > > + ufs_pm_lvl_states[lvl].link_state)); > > > + > > > > sysfs if "one value per file", not "random text that someone has to parse per > > file" please. > > > > Huge hint, if you ever care about checking the size of the sysfs buffer you are > > writing into, you are doing something really really wrong. > > > Hi Greg > It's the existing code, added by: > commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12 > Author: subhashj@codeaurora.org <subhashj@codeaurora.org> > Date: Thu Dec 22 18:41:00 2016 -0800 > > scsi: ufs: provide sysfs attribute to select the PM level > > This patch provides the sysfs attribute to choose the power management > level for UFS runtime and system suspend. > > Reviewed-by: Sujit Reddy Thumma <sthumma@codeaurora.org> > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > I just moved it to an another file and changed the sysfs entries creation by > Jaegeuk Kim' request. At the moment the entry shows the PM level, the device > state, the link state and all possible PM levels. Do you want me to change it? Ah, you are just moving this code around. Ok, that's fine for this patch, but please fix it up as part of this patch series because this isn't an acceptable sysfs file at all. If it were documented that would be a lot more obvious as to just how wrong it was :( And, as it wasn't documented, you can change it as it's obvious no one used it :) thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Sunday, February 4, 2018 2:34 PM > To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com> > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > jaegeuk@kernel.org; Alex Lemberg <Alex.Lemberg@wdc.com> > Subject: Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs > entries. > > On Sun, Feb 04, 2018 at 12:29:06PM +0000, Stanislav Nijnikov wrote: > > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > > > + "\nAll available Runtime PM levels info:\n"); > > > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > > > + "\tRuntime PM level [%d] => dev_state > > > [%s] link_state [%s]\n", > > > > + lvl, > > > > + ufschd_ufs_dev_pwr_mode_to_string( > > > > + ufs_pm_lvl_states[lvl].dev_state), > > > > + ufschd_uic_link_state_to_string( > > > > + ufs_pm_lvl_states[lvl].link_state)); > > > > + > > > > > > sysfs if "one value per file", not "random text that someone has to > > > parse per file" please. > > > > > > Huge hint, if you ever care about checking the size of the sysfs > > > buffer you are writing into, you are doing something really really wrong. > > > > > Hi Greg > > It's the existing code, added by: > > commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12 > > Author: subhashj@codeaurora.org <subhashj@codeaurora.org> > > Date: Thu Dec 22 18:41:00 2016 -0800 > > > > scsi: ufs: provide sysfs attribute to select the PM level > > > > This patch provides the sysfs attribute to choose the power management > > level for UFS runtime and system suspend. > > > > Reviewed-by: Sujit Reddy Thumma <sthumma@codeaurora.org> > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> > > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > > > I just moved it to an another file and changed the sysfs entries > > creation by Jaegeuk Kim' request. At the moment the entry shows the PM > > level, the device state, the link state and all possible PM levels. Do you > want me to change it? > > Ah, you are just moving this code around. Ok, that's fine for this patch, but > please fix it up as part of this patch series because this isn't an acceptable > sysfs file at all. If it were documented that would be a lot more obvious as to > just how wrong it was :( > > And, as it wasn't documented, you can change it as it's obvious no one used it > :) > > thanks, > > greg k-h Can I fix these entries not in this patchset? As long as I know they are used and I would prefer that change of the existing sysfs entries behavior be related to a separate patch. Regards. Stanislav.
On Sun, Feb 04, 2018 at 01:09:09PM +0000, Stanislav Nijnikov wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Sunday, February 4, 2018 2:34 PM > > To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com> > > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > > jaegeuk@kernel.org; Alex Lemberg <Alex.Lemberg@wdc.com> > > Subject: Re: [PATCH v4 01/10] ufs: sysfs: attribute group for existing sysfs > > entries. > > > > On Sun, Feb 04, 2018 at 12:29:06PM +0000, Stanislav Nijnikov wrote: > > > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > > > > + "\nAll available Runtime PM levels info:\n"); > > > > > + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) > > > > > + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), > > > > > + "\tRuntime PM level [%d] => dev_state > > > > [%s] link_state [%s]\n", > > > > > + lvl, > > > > > + ufschd_ufs_dev_pwr_mode_to_string( > > > > > + ufs_pm_lvl_states[lvl].dev_state), > > > > > + ufschd_uic_link_state_to_string( > > > > > + ufs_pm_lvl_states[lvl].link_state)); > > > > > + > > > > > > > > sysfs if "one value per file", not "random text that someone has to > > > > parse per file" please. > > > > > > > > Huge hint, if you ever care about checking the size of the sysfs > > > > buffer you are writing into, you are doing something really really wrong. > > > > > > > Hi Greg > > > It's the existing code, added by: > > > commit 09690d5a6ae1b7e4cb5ac429c311b99d09352c12 > > > Author: subhashj@codeaurora.org <subhashj@codeaurora.org> > > > Date: Thu Dec 22 18:41:00 2016 -0800 > > > > > > scsi: ufs: provide sysfs attribute to select the PM level > > > > > > This patch provides the sysfs attribute to choose the power management > > > level for UFS runtime and system suspend. > > > > > > Reviewed-by: Sujit Reddy Thumma <sthumma@codeaurora.org> > > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> > > > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > > > > > I just moved it to an another file and changed the sysfs entries > > > creation by Jaegeuk Kim' request. At the moment the entry shows the PM > > > level, the device state, the link state and all possible PM levels. Do you > > want me to change it? > > > > Ah, you are just moving this code around. Ok, that's fine for this patch, but > > please fix it up as part of this patch series because this isn't an acceptable > > sysfs file at all. If it were documented that would be a lot more obvious as to > > just how wrong it was :( > > > > And, as it wasn't documented, you can change it as it's obvious no one used it > > :) > > > > thanks, > > > > greg k-h > > Can I fix these entries not in this patchset? As long as I know they are used and I > would prefer that change of the existing sysfs entries behavior be related to a > separate patch. Ok, but it's nice to at least hope that someone will fix it up soon :) thanks, greg k-h
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile index 9310c6c..918f579 100644 --- a/drivers/scsi/ufs/Makefile +++ b/drivers/scsi/ufs/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o tc-dwc-g210.o obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-dwc-g210.o obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o +ufshcd-core-objs := ufshcd.o ufs-sysfs.o obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c new file mode 100644 index 0000000..cc68a90 --- /dev/null +++ b/drivers/scsi/ufs/ufs-sysfs.c @@ -0,0 +1,164 @@ +//SPDX-License-Identifier: GPL-2.0-only +//Copyright (C) 2018 Western Digital Corporation +//This program is free software; you can redistribute it and/or modify it +//under the terms of the GNU General Public License as published by the +//Free Software Foundation; version 2. +// +//This program is distributed in the hope that it will be useful, but +//WITHOUT ANY WARRANTY; without even the implied warranty of +//MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +//General Public License for more details. + +#include <linux/err.h> +#include <linux/string.h> + +#include "ufs-sysfs.h" + +static const char *ufschd_uic_link_state_to_string( + enum uic_link_state state) +{ + switch (state) { + case UIC_LINK_OFF_STATE: return "OFF"; + case UIC_LINK_ACTIVE_STATE: return "ACTIVE"; + case UIC_LINK_HIBERN8_STATE: return "HIBERN8"; + default: return "UNKNOWN"; + } +} + +static const char *ufschd_ufs_dev_pwr_mode_to_string( + enum ufs_dev_pwr_mode state) +{ + switch (state) { + case UFS_ACTIVE_PWR_MODE: return "ACTIVE"; + case UFS_SLEEP_PWR_MODE: return "SLEEP"; + case UFS_POWERDOWN_PWR_MODE: return "POWERDOWN"; + default: return "UNKNOWN"; + } +} + +static inline ssize_t ufs_sysfs_pm_lvl_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count, + bool rpm) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + unsigned long flags, value; + + if (kstrtoul(buf, 0, &value)) + return -EINVAL; + + if (value >= UFS_PM_LVL_MAX) + return -EINVAL; + + spin_lock_irqsave(hba->host->host_lock, flags); + if (rpm) + hba->rpm_lvl = value; + else + hba->spm_lvl = value; + spin_unlock_irqrestore(hba->host->host_lock, flags); + return count; +} + +static ssize_t rpm_lvl_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + int curr_len; + u8 lvl; + + curr_len = snprintf(buf, PAGE_SIZE, + "\nCurrent Runtime PM level [%d] => dev_state [%s] link_state [%s]\n", + hba->rpm_lvl, + ufschd_ufs_dev_pwr_mode_to_string( + ufs_pm_lvl_states[hba->rpm_lvl].dev_state), + ufschd_uic_link_state_to_string( + ufs_pm_lvl_states[hba->rpm_lvl].link_state)); + + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), + "\nAll available Runtime PM levels info:\n"); + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), + "\tRuntime PM level [%d] => dev_state [%s] link_state [%s]\n", + lvl, + ufschd_ufs_dev_pwr_mode_to_string( + ufs_pm_lvl_states[lvl].dev_state), + ufschd_uic_link_state_to_string( + ufs_pm_lvl_states[lvl].link_state)); + + return curr_len; +} + +static ssize_t rpm_lvl_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t count) +{ + return ufs_sysfs_pm_lvl_store(dev, attr, buf, count, true); +} + +static ssize_t spm_lvl_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + int curr_len; + u8 lvl; + + curr_len = snprintf(buf, PAGE_SIZE, + "\nCurrent System PM level [%d] => dev_state [%s] link_state [%s]\n", + hba->spm_lvl, + ufschd_ufs_dev_pwr_mode_to_string( + ufs_pm_lvl_states[hba->spm_lvl].dev_state), + ufschd_uic_link_state_to_string( + ufs_pm_lvl_states[hba->spm_lvl].link_state)); + + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), + "\nAll available System PM levels info:\n"); + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), + "\tSystem PM level [%d] => dev_state [%s] link_state [%s]\n", + lvl, + ufschd_ufs_dev_pwr_mode_to_string( + ufs_pm_lvl_states[lvl].dev_state), + ufschd_uic_link_state_to_string( + ufs_pm_lvl_states[lvl].link_state)); + + return curr_len; +} + +static ssize_t spm_lvl_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t count) +{ + return ufs_sysfs_pm_lvl_store(dev, attr, buf, count, false); +} + +static DEVICE_ATTR_RW(rpm_lvl); +static DEVICE_ATTR_RW(spm_lvl); + +static struct attribute *ufs_sysfs_ufshcd_attrs[] = { + &dev_attr_rpm_lvl.attr, + &dev_attr_spm_lvl.attr, + NULL +}; + +static const struct attribute_group ufs_sysfs_default_group = { + .attrs = ufs_sysfs_ufshcd_attrs, +}; + +static const struct attribute_group *ufs_sysfs_groups[] = { + &ufs_sysfs_default_group, + NULL, +}; + +void ufs_sysfs_add_nodes(struct device *dev) +{ + int ret; + + ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups); + if (ret) + dev_err(dev, + "%s: sysfs groups creation failed (err = %d)\n", + __func__, ret); +} + +void ufs_sysfs_remove_nodes(struct device *dev) +{ + sysfs_remove_groups(&dev->kobj, ufs_sysfs_groups); +} diff --git a/drivers/scsi/ufs/ufs-sysfs.h b/drivers/scsi/ufs/ufs-sysfs.h new file mode 100644 index 0000000..c53c039 --- /dev/null +++ b/drivers/scsi/ufs/ufs-sysfs.h @@ -0,0 +1,22 @@ +/*SPDX-License-Identifier: GPL-2.0-only + *Copyright (C) 2018 Western Digital Corporation + *This program is free software; you can redistribute it and/or modify it + *under the terms of the GNU General Public License as published by the + *Free Software Foundation; version 2. + * + *This program is distributed in the hope that it will be useful, but + *WITHOUT ANY WARRANTY; without even the implied warranty of + *MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + *General Public License for more details. + */ + +#ifndef __UFS_SYSFS_H__ +#define __UFS_SYSFS_H__ + +#include <linux/sysfs.h> + +#include "ufshcd.h" + +void ufs_sysfs_add_nodes(struct device *dev); +void ufs_sysfs_remove_nodes(struct device *dev); +#endif diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index a355d98..e7621a0a 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -44,6 +44,7 @@ #include "ufshcd.h" #include "ufs_quirks.h" #include "unipro.h" +#include "ufs-sysfs.h" #define CREATE_TRACE_POINTS #include <trace/events/ufs.h> @@ -150,7 +151,7 @@ enum { #define ufshcd_is_ufs_dev_poweroff(h) \ ((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE) -static struct ufs_pm_lvl_states ufs_pm_lvl_states[] = { +struct ufs_pm_lvl_states ufs_pm_lvl_states[] = { {UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE}, {UFS_ACTIVE_PWR_MODE, UIC_LINK_HIBERN8_STATE}, {UFS_SLEEP_PWR_MODE, UIC_LINK_ACTIVE_STATE}, @@ -813,28 +814,6 @@ static inline bool ufshcd_is_hba_active(struct ufs_hba *hba) ? false : true; } -static const char *ufschd_uic_link_state_to_string( - enum uic_link_state state) -{ - switch (state) { - case UIC_LINK_OFF_STATE: return "OFF"; - case UIC_LINK_ACTIVE_STATE: return "ACTIVE"; - case UIC_LINK_HIBERN8_STATE: return "HIBERN8"; - default: return "UNKNOWN"; - } -} - -static const char *ufschd_ufs_dev_pwr_mode_to_string( - enum ufs_dev_pwr_mode state) -{ - switch (state) { - case UFS_ACTIVE_PWR_MODE: return "ACTIVE"; - case UFS_SLEEP_PWR_MODE: return "SLEEP"; - case UFS_POWERDOWN_PWR_MODE: return "POWERDOWN"; - default: return "UNKNOWN"; - } -} - u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba) { /* HCI version 1.0 and 1.1 supports UniPro 1.41 */ @@ -7585,133 +7564,6 @@ int ufshcd_runtime_idle(struct ufs_hba *hba) } EXPORT_SYMBOL(ufshcd_runtime_idle); -static inline ssize_t ufshcd_pm_lvl_store(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t count, - bool rpm) -{ - struct ufs_hba *hba = dev_get_drvdata(dev); - unsigned long flags, value; - - if (kstrtoul(buf, 0, &value)) - return -EINVAL; - - if (value >= UFS_PM_LVL_MAX) - return -EINVAL; - - spin_lock_irqsave(hba->host->host_lock, flags); - if (rpm) - hba->rpm_lvl = value; - else - hba->spm_lvl = value; - spin_unlock_irqrestore(hba->host->host_lock, flags); - return count; -} - -static ssize_t ufshcd_rpm_lvl_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct ufs_hba *hba = dev_get_drvdata(dev); - int curr_len; - u8 lvl; - - curr_len = snprintf(buf, PAGE_SIZE, - "\nCurrent Runtime PM level [%d] => dev_state [%s] link_state [%s]\n", - hba->rpm_lvl, - ufschd_ufs_dev_pwr_mode_to_string( - ufs_pm_lvl_states[hba->rpm_lvl].dev_state), - ufschd_uic_link_state_to_string( - ufs_pm_lvl_states[hba->rpm_lvl].link_state)); - - curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), - "\nAll available Runtime PM levels info:\n"); - for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) - curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), - "\tRuntime PM level [%d] => dev_state [%s] link_state [%s]\n", - lvl, - ufschd_ufs_dev_pwr_mode_to_string( - ufs_pm_lvl_states[lvl].dev_state), - ufschd_uic_link_state_to_string( - ufs_pm_lvl_states[lvl].link_state)); - - return curr_len; -} - -static ssize_t ufshcd_rpm_lvl_store(struct device *dev, - struct device_attribute *attr, const char *buf, size_t count) -{ - return ufshcd_pm_lvl_store(dev, attr, buf, count, true); -} - -static void ufshcd_add_rpm_lvl_sysfs_nodes(struct ufs_hba *hba) -{ - hba->rpm_lvl_attr.show = ufshcd_rpm_lvl_show; - hba->rpm_lvl_attr.store = ufshcd_rpm_lvl_store; - sysfs_attr_init(&hba->rpm_lvl_attr.attr); - hba->rpm_lvl_attr.attr.name = "rpm_lvl"; - hba->rpm_lvl_attr.attr.mode = 0644; - if (device_create_file(hba->dev, &hba->rpm_lvl_attr)) - dev_err(hba->dev, "Failed to create sysfs for rpm_lvl\n"); -} - -static ssize_t ufshcd_spm_lvl_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct ufs_hba *hba = dev_get_drvdata(dev); - int curr_len; - u8 lvl; - - curr_len = snprintf(buf, PAGE_SIZE, - "\nCurrent System PM level [%d] => dev_state [%s] link_state [%s]\n", - hba->spm_lvl, - ufschd_ufs_dev_pwr_mode_to_string( - ufs_pm_lvl_states[hba->spm_lvl].dev_state), - ufschd_uic_link_state_to_string( - ufs_pm_lvl_states[hba->spm_lvl].link_state)); - - curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), - "\nAll available System PM levels info:\n"); - for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++) - curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len), - "\tSystem PM level [%d] => dev_state [%s] link_state [%s]\n", - lvl, - ufschd_ufs_dev_pwr_mode_to_string( - ufs_pm_lvl_states[lvl].dev_state), - ufschd_uic_link_state_to_string( - ufs_pm_lvl_states[lvl].link_state)); - - return curr_len; -} - -static ssize_t ufshcd_spm_lvl_store(struct device *dev, - struct device_attribute *attr, const char *buf, size_t count) -{ - return ufshcd_pm_lvl_store(dev, attr, buf, count, false); -} - -static void ufshcd_add_spm_lvl_sysfs_nodes(struct ufs_hba *hba) -{ - hba->spm_lvl_attr.show = ufshcd_spm_lvl_show; - hba->spm_lvl_attr.store = ufshcd_spm_lvl_store; - sysfs_attr_init(&hba->spm_lvl_attr.attr); - hba->spm_lvl_attr.attr.name = "spm_lvl"; - hba->spm_lvl_attr.attr.mode = 0644; - if (device_create_file(hba->dev, &hba->spm_lvl_attr)) - dev_err(hba->dev, "Failed to create sysfs for spm_lvl\n"); -} - -static inline void ufshcd_add_sysfs_nodes(struct ufs_hba *hba) -{ - ufshcd_add_rpm_lvl_sysfs_nodes(hba); - ufshcd_add_spm_lvl_sysfs_nodes(hba); -} - -static inline void ufshcd_remove_sysfs_nodes(struct ufs_hba *hba) -{ - device_remove_file(hba->dev, &hba->rpm_lvl_attr); - device_remove_file(hba->dev, &hba->spm_lvl_attr); -} - /** * ufshcd_shutdown - shutdown routine * @hba: per adapter instance @@ -7749,7 +7601,7 @@ EXPORT_SYMBOL(ufshcd_shutdown); */ void ufshcd_remove(struct ufs_hba *hba) { - ufshcd_remove_sysfs_nodes(hba); + ufs_sysfs_remove_nodes(hba->dev); scsi_remove_host(hba->host); /* disable interrupts */ ufshcd_disable_intr(hba, hba->intr_mask); @@ -7996,7 +7848,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) ufshcd_set_ufs_dev_active(hba); async_schedule(ufshcd_async_scan, hba); - ufshcd_add_sysfs_nodes(hba); + ufs_sysfs_add_nodes(hba->dev); return 0; diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 1332e54..53e2779 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -985,4 +985,6 @@ static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba) hba->vops->dbg_register_dump(hba); } +extern struct ufs_pm_lvl_states ufs_pm_lvl_states[]; + #endif /* End of Header */
This patch introduces attribute group to show existing sysfs entries. Signed-off-by: Stanislav Nijnikov <stanislav.nijnikov@wdc.com> --- drivers/scsi/ufs/Makefile | 3 +- drivers/scsi/ufs/ufs-sysfs.c | 164 +++++++++++++++++++++++++++++++++++++++++++ drivers/scsi/ufs/ufs-sysfs.h | 22 ++++++ drivers/scsi/ufs/ufshcd.c | 156 ++-------------------------------------- drivers/scsi/ufs/ufshcd.h | 2 + 5 files changed, 194 insertions(+), 153 deletions(-) create mode 100644 drivers/scsi/ufs/ufs-sysfs.c create mode 100644 drivers/scsi/ufs/ufs-sysfs.h