| 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
| ||
|
Message-ID: <3a9dfc05-f6c3-a283-5791-5d9b72bb44f7@st.com>
Date: Fri, 14 Feb 2020 15:50:57 +0100
From: Fabrice Gasnier <fabrice.gasnier@...com>
To: Jonathan Cameron <jic23@...nel.org>
CC: <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <mcoquelin.stm32@...il.com>,
<benjamin.gaignard@...com>, <alexandre.torgue@...com>,
<olivier.moysan@...com>, <linux-iio@...r.kernel.org>,
<lars@...afoo.de>, <knaack.h@....de>, <pmeerw@...erw.net>,
<linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master
mode
On 2/14/20 3:20 PM, Jonathan Cameron wrote:
> On Fri, 7 Feb 2020 17:30:31 +0100
> Fabrice Gasnier <fabrice.gasnier@...com> wrote:
>
>> Clock should be enabled as soon as using master modes, even before
>> enabling timer. Or, this may provoke bad behavior on the other end
>> (slave timer). Then, introduce 'clk_enabled' flag, instead of relying
>> on CR1 EN bit, to keep track of clock being enabled (balanced refcount).
>> Propagate this anywhere else in the driver.
>>
>> Also add 'remove' routine to stop timer and disable clock in case it
>> has been left enabled. Enforce the user interface has been unregistered
>> in the remove routine, before disabling the hardware to avoid possible
>> race. So, remove use of devm_ variant to register triggers and unregister
>> them before the hardware gets disabled [1].
>> [1] https://patchwork.kernel.org/patch/9956247/
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
> Patch looks fine. So is this a fix for a known issue or a theoretical one?
> Just a question of whether to line it up for an rc, or wait for next
> merge window. Ideally please give me a fixes tag as well if this
> fixes a problem that is being seen in the wild.
Hi Jonathan,
I got no complain for now. So I think this is fine to wait for the next
merge window.
Thanks,
Fabrice
>
> Thanks,
>
> Jonathan
>
>> ---
>> Changes in v2:
>> - enforce the user interface has been unregistered in the remove routine,
>> before disabling the hardware to avoid possible race.
>> ---
>> drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++-------
>> 1 file changed, 76 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
>> index a5dfe65..473853e 100644
>> --- a/drivers/iio/trigger/stm32-timer-trigger.c
>> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
>> @@ -79,10 +79,13 @@ struct stm32_timer_trigger {
>> struct device *dev;
>> struct regmap *regmap;
>> struct clk *clk;
>> + bool clk_enabled;
>> u32 max_arr;
>> const void *triggers;
>> const void *valids;
>> bool has_trgo2;
>> + struct mutex lock; /* concurrent sysfs configuration */
>> + struct list_head tr_list;
>> };
>>
>> struct stm32_timer_trigger_cfg {
>> @@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>> {
>> unsigned long long prd, div;
>> int prescaler = 0;
>> - u32 ccer, cr1;
>> + u32 ccer;
>>
>> /* Period and prescaler values depends of clock rate */
>> div = (unsigned long long)clk_get_rate(priv->clk);
>> @@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>> if (ccer & TIM_CCER_CCXE)
>> return -EBUSY;
>>
>> - regmap_read(priv->regmap, TIM_CR1, &cr1);
>> - if (!(cr1 & TIM_CR1_CEN))
>> + mutex_lock(&priv->lock);
>> + if (!priv->clk_enabled) {
>> + priv->clk_enabled = true;
>> clk_enable(priv->clk);
>> + }
>>
>> regmap_write(priv->regmap, TIM_PSC, prescaler);
>> regmap_write(priv->regmap, TIM_ARR, prd - 1);
>> @@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>
>> /* Enable controller */
>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>> + mutex_unlock(&priv->lock);
>>
>> return 0;
>> }
>>
>> static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>> {
>> - u32 ccer, cr1;
>> + u32 ccer;
>>
>> regmap_read(priv->regmap, TIM_CCER, &ccer);
>> if (ccer & TIM_CCER_CCXE)
>> return;
>>
>> - regmap_read(priv->regmap, TIM_CR1, &cr1);
>> - if (cr1 & TIM_CR1_CEN)
>> - clk_disable(priv->clk);
>> -
>> + mutex_lock(&priv->lock);
>> /* Stop timer */
>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> @@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>
>> /* Make sure that registers are updated */
>> regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>> +
>> + if (priv->clk_enabled) {
>> + priv->clk_enabled = false;
>> + clk_disable(priv->clk);
>> + }
>> + mutex_unlock(&priv->lock);
>> }
>>
>> static ssize_t stm32_tt_store_frequency(struct device *dev,
>> @@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
>> for (i = 0; i <= master_mode_max; i++) {
>> if (!strncmp(master_mode_table[i], buf,
>> strlen(master_mode_table[i]))) {
>> + mutex_lock(&priv->lock);
>> + if (!priv->clk_enabled) {
>> + /* Clock should be enabled first */
>> + priv->clk_enabled = true;
>> + clk_enable(priv->clk);
>> + }
>> regmap_update_bits(priv->regmap, TIM_CR2, mask,
>> i << shift);
>> /* Make sure that registers are updated */
>> regmap_update_bits(priv->regmap, TIM_EGR,
>> TIM_EGR_UG, TIM_EGR_UG);
>> + mutex_unlock(&priv->lock);
>> return len;
>> }
>> }
>> @@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = {
>> static const struct iio_trigger_ops timer_trigger_ops = {
>> };
>>
>> -static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>> +static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv)
>> +{
>> + struct iio_trigger *tr;
>> +
>> + list_for_each_entry(tr, &priv->tr_list, alloc_list)
>> + iio_trigger_unregister(tr);
>> +}
>> +
>> +static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv)
>> {
>> int ret;
>> const char * const *cur = priv->triggers;
>>
>> + INIT_LIST_HEAD(&priv->tr_list);
>> +
>> while (cur && *cur) {
>> struct iio_trigger *trig;
>> bool cur_is_trgo = stm32_timer_is_trgo_name(*cur);
>> @@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>>
>> iio_trigger_set_drvdata(trig, priv);
>>
>> - ret = devm_iio_trigger_register(priv->dev, trig);
>> - if (ret)
>> + ret = iio_trigger_register(trig);
>> + if (ret) {
>> + stm32_unregister_iio_triggers(priv);
>> return ret;
>> + }
>> +
>> + list_add_tail(&trig->alloc_list, &priv->tr_list);
>> cur++;
>> }
>>
>> @@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>> int val, int val2, long mask)
>> {
>> struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>> - u32 dat;
>>
>> switch (mask) {
>> case IIO_CHAN_INFO_RAW:
>> @@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>> return -EINVAL;
>>
>> case IIO_CHAN_INFO_ENABLE:
>> + mutex_lock(&priv->lock);
>> if (val) {
>> - regmap_read(priv->regmap, TIM_CR1, &dat);
>> - if (!(dat & TIM_CR1_CEN))
>> + if (!priv->clk_enabled) {
>> + priv->clk_enabled = true;
>> clk_enable(priv->clk);
>> + }
>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>> TIM_CR1_CEN);
>> } else {
>> - regmap_read(priv->regmap, TIM_CR1, &dat);
>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>> 0);
>> - if (dat & TIM_CR1_CEN)
>> + if (priv->clk_enabled) {
>> + priv->clk_enabled = false;
>> clk_disable(priv->clk);
>> + }
>> }
>> + mutex_unlock(&priv->lock);
>> return 0;
>> }
>>
>> @@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>> {
>> struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>> int sms = stm32_enable_mode2sms(mode);
>> - u32 val;
>>
>> if (sms < 0)
>> return sms;
>> @@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>> * Triggered mode sets CEN bit automatically by hardware. So, first
>> * enable counter clock, so it can use it. Keeps it in sync with CEN.
>> */
>> - if (sms == 6) {
>> - regmap_read(priv->regmap, TIM_CR1, &val);
>> - if (!(val & TIM_CR1_CEN))
>> - clk_enable(priv->clk);
>> + mutex_lock(&priv->lock);
>> + if (sms == 6 && !priv->clk_enabled) {
>> + clk_enable(priv->clk);
>> + priv->clk_enabled = true;
>> }
>> + mutex_unlock(&priv->lock);
>>
>> regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
>>
>> @@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>> priv->triggers = triggers_table[index];
>> priv->valids = cfg->valids_table[index];
>> stm32_timer_detect_trgo2(priv);
>> + mutex_init(&priv->lock);
>>
>> - ret = stm32_setup_iio_triggers(priv);
>> + ret = stm32_register_iio_triggers(priv);
>> if (ret)
>> return ret;
>>
>> @@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static int stm32_timer_trigger_remove(struct platform_device *pdev)
>> +{
>> + struct stm32_timer_trigger *priv = platform_get_drvdata(pdev);
>> + u32 val;
>> +
>> + /* Unregister triggers before everything can be safely turned off */
>> + stm32_unregister_iio_triggers(priv);
>> +
>> + /* Check if nobody else use the timer, then disable it */
>> + regmap_read(priv->regmap, TIM_CCER, &val);
>> + if (!(val & TIM_CCER_CCXE))
>> + regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> +
>> + if (priv->clk_enabled)
>> + clk_disable(priv->clk);
>> +
>> + return 0;
>> +}
>> +
>> static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = {
>> .valids_table = valids_table,
>> .num_valids_table = ARRAY_SIZE(valids_table),
>> @@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>>
>> static struct platform_driver stm32_timer_trigger_driver = {
>> .probe = stm32_timer_trigger_probe,
>> + .remove = stm32_timer_trigger_remove,
>> .driver = {
>> .name = "stm32-timer-trigger",
>> .of_match_table = stm32_trig_of_match,
>
Powered by blists - more mailing lists