| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 22 Feb 2022 14:19:12 +0800
From: Song Chen <chensong_2000@....cn>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: johan@...nel.org, elder@...nel.org, thierry.reding@...il.com,
u.kleine-koenig@...gutronix.de, lee.jones@...aro.org,
greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2] staging: greybus: introduce pwm_ops::apply
Hi Greg,
在 2022/2/22 01:06, Greg KH 写道:
> On Fri, Feb 11, 2022 at 08:02:27PM +0800, Song Chen wrote:
>> Introduce apply in pwm_ops to replace legacy operations,
>> like enable, disable, config and set_polarity.
>>
>> Signed-off-by: Song Chen <chensong_2000@....cn>
>>
>> ---
>> V2:
>> 1, define duty_cycle and period as u64 in gb_pwm_config_operation.
>> 2, define duty and period as u64 in gb_pwm_config_request.
>> 3, disable before configuring duty and period if the eventual goal
>> is a disabled state.
>> ---
>> drivers/staging/greybus/pwm.c | 61 ++++++++++++-----------
>> include/linux/greybus/greybus_protocols.h | 4 +-
>> 2 files changed, 34 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
>> index 891a6a672378..03c69db5b9be 100644
>> --- a/drivers/staging/greybus/pwm.c
>> +++ b/drivers/staging/greybus/pwm.c
>> @@ -89,7 +89,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
>> }
>>
>> static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
>> - u8 which, u32 duty, u32 period)
>> + u8 which, u64 duty, u64 period)
>> {
>> struct gb_pwm_config_request request;
>> struct gbphy_device *gbphy_dev;
>> @@ -99,8 +99,8 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
>> return -EINVAL;
>>
>> request.which = which;
>> - request.duty = cpu_to_le32(duty);
>> - request.period = cpu_to_le32(period);
>> + request.duty = duty;
>> + request.period = period;
>>
>> gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
>> ret = gbphy_runtime_get_sync(gbphy_dev);
>> @@ -204,43 +204,46 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
>> }
>>
>> -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> - int duty_ns, int period_ns)
>> +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> {
>> + int err;
>> + bool enabled = pwm->state.enabled;
>> struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>
>> - return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
>> -};
>> -
>> -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> - enum pwm_polarity polarity)
>> -{
>> - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>> -
>> - return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
>> -};
>> + /* set polarity */
>> + if (state->polarity != pwm->state.polarity) {
>> + if (enabled) {
>> + gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>> + enabled = false;
>> + }
>> + err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
>> + if (err)
>> + return err;
>> + }
>>
>> -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> -{
>> - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>> + if (!state->enabled) {
>> + if (enabled)
>> + gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>> + return 0;
>> + }
>>
>> - return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>> -};
>> + /* set period and duty cycle*/
>> + err = gb_pwm_config_operation(pwmc, pwm->hwpwm, state->duty_cycle, state->period);
>> + if (err)
>> + return err;
>>
>> -static void gb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> -{
>> - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>> + /* enable/disable */
>> + if (!enabled)
>> + return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>>
>> - gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>> -};
>> + return 0;
>> +}
>>
>> static const struct pwm_ops gb_pwm_ops = {
>> .request = gb_pwm_request,
>> .free = gb_pwm_free,
>> - .config = gb_pwm_config,
>> - .set_polarity = gb_pwm_set_polarity,
>> - .enable = gb_pwm_enable,
>> - .disable = gb_pwm_disable,
>> + .apply = gb_pwm_apply,
>> .owner = THIS_MODULE,
>> };
>>
>> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
>> index aeb8f9243545..81a6f16de098 100644
>> --- a/include/linux/greybus/greybus_protocols.h
>> +++ b/include/linux/greybus/greybus_protocols.h
>> @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
>>
>> struct gb_pwm_config_request {
>> __u8 which;
>> - __le32 duty;
>> - __le32 period;
>> + __u64 duty;
>> + __u64 period;
>> } __packed;
>
> Did you just change a greybus protocol message that is sent over the
> wire? Why? And why drop the endian marking of it?
I discussed with Uwe about losing bit and found there is no perfect way
to avoid, even in Uwe's patch[1], as a result, we decided to widen duty
and period in gb_pwm_config_request, the other side of the wire is
supposed to change accordingly to support 64bit duty and period too(this
will introduce compatibility problem and there is no variable like
version to address it), similar with ktime_t in y2038, below is the
detail [2]
[1]:https://lore.kernel.org/all/20210312212119.1342666-1-u.kleine-koenig@pengutronix.de/
[2]:https://lore.kernel.org/all/20220211071601.4rpfbkit6c6dre2o@pengutronix.de/
endian is a problem, i shouldn't drop it.
BR
Song
>
> Are you sure this is ok?
>
> thanks,
>
> greg k-h
>
Powered by blists - more mailing lists