[PATCH v2 1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr()

[PATCH v2 1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr()

[Andy Shevchenko]

Using these newer macros allows the compiler to remove the unused
structure and functions when !CONFIG_PM_SLEEP + removes the need to
mark pm functions __maybe_unused.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
v2: added tag (Uwe)
 drivers/pwm/sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 9903c3a7eced..c21b6046067b 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -433,7 +433,7 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
 	return ret;
 }
 
-static int __maybe_unused pwm_class_suspend(struct device *parent)
+static int pwm_class_suspend(struct device *parent)
 {
 	struct pwm_chip *chip = dev_get_drvdata(parent);
 	unsigned int i;
@@ -464,20 +464,20 @@ static int __maybe_unused pwm_class_suspend(struct device *parent)
 	return ret;
 }
 
-static int __maybe_unused pwm_class_resume(struct device *parent)
+static int pwm_class_resume(struct device *parent)
 {
 	struct pwm_chip *chip = dev_get_drvdata(parent);
 
 	return pwm_class_resume_npwm(parent, chip->npwm);
 }
 
-static SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume);
 
 static struct class pwm_class = {
 	.name = "pwm",
 	.owner = THIS_MODULE,
 	.dev_groups = pwm_chip_groups,
-	.pm = &pwm_class_pm_ops,
+	.pm = pm_sleep_ptr(&pwm_class_pm_ops),
 };
 
 static int pwmchip_sysfs_match(struct device *parent, const void *data)
-- 
2.35.1

[PATCH v2 2/4] pwm: sysfs: Return directly from the for-loop in PM callbacks

[Andy Shevchenko]

There is no need to assign ret to 0 and then break the loop just
for returning the error to the caller. Instead, return directly
from the for-loop, and 0 otherwise.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
v2: added tag (Uwe)
 drivers/pwm/sysfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index c21b6046067b..1543fe07765b 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -413,7 +413,7 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
 {
 	struct pwm_chip *chip = dev_get_drvdata(parent);
 	unsigned int i;
-	int ret = 0;
+	int ret;
 
 	for (i = 0; i < npwm; i++) {
 		struct pwm_device *pwm = &chip->pwms[i];
@@ -427,17 +427,17 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
 		state.enabled = export->suspend.enabled;
 		ret = pwm_class_apply_state(export, pwm, &state);
 		if (ret < 0)
-			break;
+			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int pwm_class_suspend(struct device *parent)
 {
 	struct pwm_chip *chip = dev_get_drvdata(parent);
 	unsigned int i;
-	int ret = 0;
+	int ret;
 
 	for (i = 0; i < chip->npwm; i++) {
 		struct pwm_device *pwm = &chip->pwms[i];
@@ -457,11 +457,11 @@ static int pwm_class_suspend(struct device *parent)
 			 * this suspend function.
 			 */
 			pwm_class_resume_npwm(parent, i);
-			break;
+			return ret;
 		}
 	}
 
-	return ret;
+	return 0;
 }
 
 static int pwm_class_resume(struct device *parent)
-- 
2.35.1

[PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Andy Shevchenko]

For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
instead of the raw sprintf() & co. This patch replaces such a
sprintf() call straightforwardly with the new helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
v2: added tag (Uwe)
 drivers/pwm/sysfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 1543fe07765b..767c4b19afb1 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -42,7 +42,7 @@ static ssize_t period_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%llu\n", state.period);
+	return sysfs_emit(buf, "%llu\n", state.period);
 }
 
 static ssize_t period_store(struct device *child,
@@ -77,7 +77,7 @@ static ssize_t duty_cycle_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%llu\n", state.duty_cycle);
+	return sysfs_emit(buf, "%llu\n", state.duty_cycle);
 }
 
 static ssize_t duty_cycle_store(struct device *child,
@@ -112,7 +112,7 @@ static ssize_t enable_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%d\n", state.enabled);
+	return sysfs_emit(buf, "%d\n", state.enabled);
 }
 
 static ssize_t enable_store(struct device *child,
@@ -171,7 +171,7 @@ static ssize_t polarity_show(struct device *child,
 		break;
 	}
 
-	return sprintf(buf, "%s\n", polarity);
+	return sysfs_emit(buf, "%s\n", polarity);
 }
 
 static ssize_t polarity_store(struct device *child,
@@ -212,7 +212,7 @@ static ssize_t capture_show(struct device *child,
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
+	return sysfs_emit(buf, "%u %u\n", result.period, result.duty_cycle);
 }
 
 static DEVICE_ATTR_RW(period);
@@ -361,7 +361,7 @@ static ssize_t npwm_show(struct device *parent, struct device_attribute *attr,
 {
 	const struct pwm_chip *chip = dev_get_drvdata(parent);
 
-	return sprintf(buf, "%u\n", chip->npwm);
+	return sysfs_emit(buf, "%u\n", chip->npwm);
 }
 static DEVICE_ATTR_RO(npwm);
 
-- 
2.35.1

[PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Andy Shevchenko]

Code is smaller and looks nicer if we combine polarity strings
into an array.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: added pwm_ prefix to the variable (Uwe), adjusted intendation (Uwe)
 drivers/pwm/sysfs.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 767c4b19afb1..502167e44a3d 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -151,27 +151,23 @@ static ssize_t enable_store(struct device *child,
 	return ret ? : size;
 }
 
+static const char * const pwm_polarity_strings[] = {
+	[PWM_POLARITY_NORMAL] = "normal",
+	[PWM_POLARITY_INVERSED] = "inversed",
+};
+
 static ssize_t polarity_show(struct device *child,
 			     struct device_attribute *attr,
 			     char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
-	const char *polarity = "unknown";
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
+	if (state.polarity < 0 || state.polarity >= ARRAY_SIZE(pwm_polarity_strings))
+		return sysfs_emit(buf, "unknown\n");
 
-	switch (state.polarity) {
-	case PWM_POLARITY_NORMAL:
-		polarity = "normal";
-		break;
-
-	case PWM_POLARITY_INVERSED:
-		polarity = "inversed";
-		break;
-	}
-
-	return sysfs_emit(buf, "%s\n", polarity);
+	return sysfs_emit(buf, "%s\n", pwm_polarity_strings[state.polarity]);
 }
 
 static ssize_t polarity_store(struct device *child,
@@ -180,20 +176,16 @@ static ssize_t polarity_store(struct device *child,
 {
 	struct pwm_export *export = child_to_pwm_export(child);
 	struct pwm_device *pwm = export->pwm;
-	enum pwm_polarity polarity;
 	struct pwm_state state;
 	int ret;
 
-	if (sysfs_streq(buf, "normal"))
-		polarity = PWM_POLARITY_NORMAL;
-	else if (sysfs_streq(buf, "inversed"))
-		polarity = PWM_POLARITY_INVERSED;
-	else
-		return -EINVAL;
+	ret = sysfs_match_string(pwm_polarity_strings, buf);
+	if (ret < 0)
+		return ret;
 
 	mutex_lock(&export->lock);
 	pwm_get_state(pwm, &state);
-	state.polarity = polarity;
+	state.polarity = ret;
 	ret = pwm_apply_state(pwm, &state);
 	mutex_unlock(&export->lock);
 
-- 
2.35.1

Re: [PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Joe Perches]

On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> Code is smaller and looks nicer if we combine polarity strings
> into an array.

It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
are now required to be 0 and 1.  As the only 2 values in
an enum they are, but that's not really guaranteed unless
you read the enum definition.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: added pwm_ prefix to the variable (Uwe), adjusted intendation (Uwe)
>  drivers/pwm/sysfs.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 767c4b19afb1..502167e44a3d 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -151,27 +151,23 @@ static ssize_t enable_store(struct device *child,
>  	return ret ? : size;
>  }
>  
> +static const char * const pwm_polarity_strings[] = {
> +	[PWM_POLARITY_NORMAL] = "normal",
> +	[PWM_POLARITY_INVERSED] = "inversed",
> +};
> +
>  static ssize_t polarity_show(struct device *child,
>  			     struct device_attribute *attr,
>  			     char *buf)
>  {
>  	const struct pwm_device *pwm = child_to_pwm_device(child);
> -	const char *polarity = "unknown";
>  	struct pwm_state state;
>  
>  	pwm_get_state(pwm, &state);
> +	if (state.polarity < 0 || state.polarity >= ARRAY_SIZE(pwm_polarity_strings))
> +		return sysfs_emit(buf, "unknown\n");
>  
> -	switch (state.polarity) {
> -	case PWM_POLARITY_NORMAL:
> -		polarity = "normal";
> -		break;
> -
> -	case PWM_POLARITY_INVERSED:
> -		polarity = "inversed";
> -		break;
> -	}
> -
> -	return sysfs_emit(buf, "%s\n", polarity);
> +	return sysfs_emit(buf, "%s\n", pwm_polarity_strings[state.polarity]);
>  }
>  
>  static ssize_t polarity_store(struct device *child,
> @@ -180,20 +176,16 @@ static ssize_t polarity_store(struct device *child,
>  {
>  	struct pwm_export *export = child_to_pwm_export(child);
>  	struct pwm_device *pwm = export->pwm;
> -	enum pwm_polarity polarity;
>  	struct pwm_state state;
>  	int ret;
>  
> -	if (sysfs_streq(buf, "normal"))
> -		polarity = PWM_POLARITY_NORMAL;
> -	else if (sysfs_streq(buf, "inversed"))
> -		polarity = PWM_POLARITY_INVERSED;
> -	else
> -		return -EINVAL;
> +	ret = sysfs_match_string(pwm_polarity_strings, buf);
> +	if (ret < 0)
> +		return ret;
>  
>  	mutex_lock(&export->lock);
>  	pwm_get_state(pwm, &state);
> -	state.polarity = polarity;
> +	state.polarity = ret;
>  	ret = pwm_apply_state(pwm, &state);
>  	mutex_unlock(&export->lock);
>  

Re: [PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Joe Perches]

On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> On Sunday, August 28, 2022, Joe Perches <joe@perches.com> wrote:
> 
> > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > Code is smaller and looks nicer if we combine polarity strings
> > > into an array.
> 
> First of all, please remove unnecessary context when replying.

I am _very_ aware of context.
I specifically left the code in.

> > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > are now required to be 0 and 1.  As the only 2 values in
> > an enum they are, but that's not really guaranteed unless
> > you read the enum definition.
> 
> So, what do you suggest here and in many other similar places (yes, ABI
> implied) in the kernel?

Leaving the code alone.

Re: [PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Andy Shevchenko]

On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <joe@perches.com> wrote:
> On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > On Sunday, August 28, 2022, Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > Code is smaller and looks nicer if we combine polarity strings
> > > > into an array.

> > First of all, please remove unnecessary context when replying.
>
> I am _very_ aware of context.
> I specifically left the code in.
>
> > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > are now required to be 0 and 1.  As the only 2 values in
> > > an enum they are, but that's not really guaranteed unless
> > > you read the enum definition.
> >
> > So, what do you suggest here and in many other similar places (yes, ABI
> > implied) in the kernel?
>
> Leaving the code alone.

It's good that PWM maintainers look at this differently.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Joe Perches]

On Sun, 2022-08-28 at 20:40 +0300, Andy Shevchenko wrote:
> On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <joe@perches.com> wrote:
> > On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > > On Sunday, August 28, 2022, Joe Perches <joe@perches.com> wrote:
> > > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > > Code is smaller and looks nicer if we combine polarity strings
> > > > > into an array.
> 
> > > First of all, please remove unnecessary context when replying.
> > 
> > I am _very_ aware of context.
> > I specifically left the code in.
> > 
> > > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > > are now required to be 0 and 1.  As the only 2 values in
> > > > an enum they are, but that's not really guaranteed unless
> > > > you read the enum definition.
> > > 
> > > So, what do you suggest here and in many other similar places (yes, ABI
> > > implied) in the kernel?
> > 
> > Leaving the code alone.
> 
> It's good that PWM maintainers look at this differently.

The enum is not userspace so it's not ABI.

The PWM maintainers are free to do what they want but I
prefer obviousness over compactness.

Re: [PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Andy Shevchenko]

On Sun, Aug 28, 2022 at 9:19 PM Joe Perches <joe@perches.com> wrote:
> On Sun, 2022-08-28 at 20:40 +0300, Andy Shevchenko wrote:
> > On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <joe@perches.com> wrote:
> > > On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > > > On Sunday, August 28, 2022, Joe Perches <joe@perches.com> wrote:
> > > > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > > > Code is smaller and looks nicer if we combine polarity strings
> > > > > > into an array.
> >
> > > > First of all, please remove unnecessary context when replying.
> > >
> > > I am _very_ aware of context.
> > > I specifically left the code in.
> > >
> > > > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > > > are now required to be 0 and 1.  As the only 2 values in
> > > > > an enum they are, but that's not really guaranteed unless
> > > > > you read the enum definition.
> > > >
> > > > So, what do you suggest here and in many other similar places (yes, ABI
> > > > implied) in the kernel?
> > >
> > > Leaving the code alone.
> >
> > It's good that PWM maintainers look at this differently.
>
> The enum is not userspace so it's not ABI.
>
> The PWM maintainers are free to do what they want but I
> prefer obviousness over compactness.

Why do you not start "fixing" other similar places in the kernel?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr()

[Andy Shevchenko]

On Fri, Aug 26, 2022 at 08:07:13PM +0300, Andy Shevchenko wrote:
> Using these newer macros allows the compiler to remove the unused
> structure and functions when !CONFIG_PM_SLEEP + removes the need to
> mark pm functions __maybe_unused.

Anything I should have done for this to be applied?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] pwm: sysfs: Utilize an array for polarity strings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On Sun, Aug 28, 2022 at 02:19:22PM -0400, Joe Perches wrote:
> On Sun, 2022-08-28 at 20:40 +0300, Andy Shevchenko wrote:
> > On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <joe@perches.com> wrote:
> > > On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > > > On Sunday, August 28, 2022, Joe Perches <joe@perches.com> wrote:
> > > > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > > > Code is smaller and looks nicer if we combine polarity strings
> > > > > > into an array.
> > 
> > > > First of all, please remove unnecessary context when replying.
> > > 
> > > I am _very_ aware of context.
> > > I specifically left the code in.
> > > 
> > > > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > > > are now required to be 0 and 1.  As the only 2 values in
> > > > > an enum they are, but that's not really guaranteed unless
> > > > > you read the enum definition.
> > > > 
> > > > So, what do you suggest here and in many other similar places (yes, ABI
> > > > implied) in the kernel?
> > > 
> > > Leaving the code alone.
> > 
> > It's good that PWM maintainers look at this differently.
> 
> The enum is not userspace so it's not ABI.
> 
> The PWM maintainers are free to do what they want but I
> prefer obviousness over compactness.

I do agree with Joe, I don't see any benefit in this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote:
> For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
> instead of the raw sprintf() & co. This patch replaces such a
> sprintf() call straightforwardly with the new helper.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> v2: added tag (Uwe)
>  drivers/pwm/sysfs.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

How exactly is sysfs_emit() safer here? In all of these cases, the
values that sprintf() writes are the only values that are written into
the buffer and we know that none of them exceed PAGE_SIZE. So the
additional checks that sysfs_emit() performs are useless.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] pwm: sysfs: Return directly from the for-loop in PM callbacks

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Fri, Aug 26, 2022 at 08:07:14PM +0300, Andy Shevchenko wrote:
> There is no need to assign ret to 0 and then break the loop just
> for returning the error to the caller. Instead, return directly
> from the for-loop, and 0 otherwise.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> v2: added tag (Uwe)
>  drivers/pwm/sysfs.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

I fail to see how this is an improvement. The outcome is exactly the
same and this doesn't even make the code shorter. Why bother?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Fri, Aug 26, 2022 at 08:07:13PM +0300, Andy Shevchenko wrote:
> Using these newer macros allows the compiler to remove the unused
> structure and functions when !CONFIG_PM_SLEEP + removes the need to
> mark pm functions __maybe_unused.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> v2: added tag (Uwe)
>  drivers/pwm/sysfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote:
> On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote:
> > For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
> > instead of the raw sprintf() & co. This patch replaces such a
> > sprintf() call straightforwardly with the new helper.

> How exactly is sysfs_emit() safer here? In all of these cases, the
> values that sprintf() writes are the only values that are written into
> the buffer and we know that none of them exceed PAGE_SIZE. So the
> additional checks that sysfs_emit() performs are useless.

This is a recommended way to use sysfs_emit() mentioned in Documentation.
Care to fix documentation?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] pwm: sysfs: Return directly from the for-loop in PM callbacks

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 02:31:23PM +0200, Thierry Reding wrote:
> On Fri, Aug 26, 2022 at 08:07:14PM +0300, Andy Shevchenko wrote:
> > There is no need to assign ret to 0 and then break the loop just
> > for returning the error to the caller. Instead, return directly
> > from the for-loop, and 0 otherwise.

> I fail to see how this is an improvement. The outcome is exactly the
> same and this doesn't even make the code shorter. Why bother?

The improvement is in maintenance. It's proven that assignments in the
definition block might lead to the subtle mistakes when it's not close
enough to the actual use. That said, this is an improvement from
maintaining and developing perspectives.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote:
> > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote:
> > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
> > > instead of the raw sprintf() & co. This patch replaces such a
> > > sprintf() call straightforwardly with the new helper.
> 
> > How exactly is sysfs_emit() safer here? In all of these cases, the
> > values that sprintf() writes are the only values that are written into
> > the buffer and we know that none of them exceed PAGE_SIZE. So the
> > additional checks that sysfs_emit() performs are useless.
> 
> This is a recommended way to use sysfs_emit() mentioned in Documentation.
> Care to fix documentation?

For your convenience, Documentation/filesystems/sysfs.rst says:

- show() should only use sysfs_emit() or sysfs_emit_at() when formatting
  the value to be returned to user space.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr()

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 02:31:45PM +0200, Thierry Reding wrote:
> On Fri, Aug 26, 2022 at 08:07:13PM +0300, Andy Shevchenko wrote:
> > Using these newer macros allows the compiler to remove the unused
> > structure and functions when !CONFIG_PM_SLEEP + removes the need to
> > mark pm functions __maybe_unused.

> Applied, thanks.

Thanks and thank you for the review. It would be nice if you can check PWM LPSS
series [1] as well and another PWM related patch [2] I sent.

[1]: https://lore.kernel.org/linux-pwm/20220927162421.11052-1-andriy.shevchenko@linux.intel.com/
[2]: https://lore.kernel.org/linux-pwm/20220927172258.62418-1-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

On Wed, Sep 28, 2022 at 04:58:17PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote:
> > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote:
> > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
> > > > instead of the raw sprintf() & co. This patch replaces such a
> > > > sprintf() call straightforwardly with the new helper.
> > 
> > > How exactly is sysfs_emit() safer here? In all of these cases, the
> > > values that sprintf() writes are the only values that are written into
> > > the buffer and we know that none of them exceed PAGE_SIZE. So the
> > > additional checks that sysfs_emit() performs are useless.
> > 
> > This is a recommended way to use sysfs_emit() mentioned in Documentation.
> > Care to fix documentation?
> 
> For your convenience, Documentation/filesystems/sysfs.rst says:
> 
> - show() should only use sysfs_emit() or sysfs_emit_at() when formatting
>   the value to be returned to user space.

Took some digging to find enough information to convince me. Again, the
commit message says that sysfs_emit() is safer, but that's a bad reason
in this case because these cases are fine. The sprintf() calls that this
replaces aren't unbound and we're not appending to an existing seq_buf,
so nothing to worry on that front.

I think the better argument for broadly applying this is to specifically
distinguish the sysfs sprintf() calls from others so that they can be
auditioned better and perhaps help with the documentation[0].

Do you mind if I apply this with a reworded documentation?

Thierry

[0]: https://lore.kernel.org/all/20200930115740.GA1611809@kroah.com/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

On Wed, Sep 28, 2022 at 05:20:07PM +0200, Thierry Reding wrote:
> On Wed, Sep 28, 2022 at 04:58:17PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote:
> > > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote:
> > > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
> > > > > instead of the raw sprintf() & co. This patch replaces such a
> > > > > sprintf() call straightforwardly with the new helper.
> > > 
> > > > How exactly is sysfs_emit() safer here? In all of these cases, the
> > > > values that sprintf() writes are the only values that are written into
> > > > the buffer and we know that none of them exceed PAGE_SIZE. So the
> > > > additional checks that sysfs_emit() performs are useless.
> > > 
> > > This is a recommended way to use sysfs_emit() mentioned in Documentation.
> > > Care to fix documentation?
> > 
> > For your convenience, Documentation/filesystems/sysfs.rst says:
> > 
> > - show() should only use sysfs_emit() or sysfs_emit_at() when formatting
> >   the value to be returned to user space.
> 
> Took some digging to find enough information to convince me. Again, the
> commit message says that sysfs_emit() is safer, but that's a bad reason
> in this case because these cases are fine. The sprintf() calls that this
> replaces aren't unbound and we're not appending to an existing seq_buf,
> so nothing to worry on that front.
> 
> I think the better argument for broadly applying this is to specifically
> distinguish the sysfs sprintf() calls from others so that they can be
> auditioned better and perhaps help with the documentation[0].
> 
> Do you mind if I apply this with a reworded documentation?

I meant "commit message", not documentation.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] pwm: sysfs: Replace sprintf() with sysfs_emit()

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 05:20:07PM +0200, Thierry Reding wrote:
> On Wed, Sep 28, 2022 at 04:58:17PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote:
> > > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote:
> > > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(),
> > > > > instead of the raw sprintf() & co. This patch replaces such a
> > > > > sprintf() call straightforwardly with the new helper.
> > > 
> > > > How exactly is sysfs_emit() safer here? In all of these cases, the
> > > > values that sprintf() writes are the only values that are written into
> > > > the buffer and we know that none of them exceed PAGE_SIZE. So the
> > > > additional checks that sysfs_emit() performs are useless.
> > > 
> > > This is a recommended way to use sysfs_emit() mentioned in Documentation.
> > > Care to fix documentation?
> > 
> > For your convenience, Documentation/filesystems/sysfs.rst says:
> > 
> > - show() should only use sysfs_emit() or sysfs_emit_at() when formatting
> >   the value to be returned to user space.
> 
> Took some digging to find enough information to convince me. Again, the
> commit message says that sysfs_emit() is safer, but that's a bad reason
> in this case because these cases are fine. The sprintf() calls that this
> replaces aren't unbound and we're not appending to an existing seq_buf,
> so nothing to worry on that front.
> 
> I think the better argument for broadly applying this is to specifically
> distinguish the sysfs sprintf() calls from others so that they can be
> auditioned better and perhaps help with the documentation[0].
> 
> Do you mind if I apply this with a reworded documentation?

I do not mind, go ahead with it.
Thank you!

> Thierry
> 
> [0]: https://lore.kernel.org/all/20200930115740.GA1611809@kroah.com/

-- 
With Best Regards,
Andy Shevchenko