| 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: <2f3e5036-caab-f892-a4ad-b852f72db331@baylibre.com>
Date: Fri, 23 Sep 2022 09:23:26 +0200
From: Julien Panis <jpanis@...libre.com>
To: William Breathitt Gray <william.gray@...aro.org>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, mranostay@...com
Subject: Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support
for ECAP
On 23/09/2022 03:08, William Breathitt Gray wrote:
> On Thu, Sep 22, 2022 at 07:04:01PM +0200, Julien Panis wrote:
>> ECAP hardware on TI AM62x SoC supports capture feature. It can be used
>> to timestamp events (falling/rising edges) detected on input signal.
>>
>> This commit adds capture driver support for ECAP hardware on AM62x SoC.
>>
>> In the ECAP hardware, capture pin can also be configured to be in
>> PWM mode. Current implementation only supports capture operating mode.
>> Hardware also supports timebase sync between multiple instances, but
>> this driver supports simple independent capture functionality.
>>
>> Signed-off-by: Julien Panis <jpanis@...libre.com>
> Hello Julien,
>
> Comments follow inline below.
>
>> +/**
>> + * struct ecap_cnt_dev - device private data structure
>> + * @enabled: device state
>> + * @clk: device clock
>> + * @regmap: device register map
>> + * @nb_ovf: number of overflows since capture start
>> + * @pm_ctx: device context for PM operations
>> + */
>> +struct ecap_cnt_dev {
>> + bool enabled;
>> + struct clk *clk;
>> + struct regmap *regmap;
>> + atomic_t nb_ovf;
>> + struct {
>> + u8 ev_mode;
>> + u32 time_cntr;
>> + } pm_ctx;
>> +};
> Provide documentation for the ev_mode and time_cntr members. You
> probably need a lock as well to protect access to this structure or
> you'll end up with race problems.
Hi William,
How can I end up with race problems ? pm_ctx members are only accessed at
suspend (after capture/IRQ are disabled) and resume (before capture/IRQ
are re-enabled).
Is there any risk I did not identify ?
Julien
>
>
>> +static void ecap_cnt_capture_enable(struct counter_device *counter)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + pm_runtime_get_sync(counter->parent);
>> +
>> + /* Enable interrupts on events */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
>> + ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK);
>> +
>> + /* Run counter */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK,
>> + ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK);
>> +}
>> +
>> +static void ecap_cnt_capture_disable(struct counter_device *counter)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + /* Disable interrupts on events */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_EVT_EN_MASK, 0);
>> +
>> + /* Stop counter */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
> Shouldn't the counter be stopped before stopping the interrupts?
>
>> +static int ecap_cnt_count_get_val(struct counter_device *counter, unsigned int reg, u32 *val)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> + unsigned int regval;
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + regmap_read(ecap_dev->regmap, reg, ®val);
>> + pm_runtime_put_sync(counter->parent);
>> +
>> + *val = regval;
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_count_set_val(struct counter_device *counter, unsigned int reg, u32 val)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + regmap_write(ecap_dev->regmap, reg, val);
>> + pm_runtime_put_sync(counter->parent);
>> +
>> + return 0;
>> +}
> The ecap_cnt_count_get_val() and ecap_cnt_count_set_val() functions only
> ever return 0. Redefine them as void functions and eliminate the
> unnecessary returns.
>
>> +static int ecap_cnt_count_write(struct counter_device *counter,
>> + struct counter_count *count, u64 val)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + if (ecap_dev->enabled)
>> + return -EBUSY;
> You should return -EBUSY when the requested operation cannot be
> completed because the device currently performing a task -- i.e. the
> requested operation would stall or otherwise fail if forced. In this
> case, the count value actually can be set while the device is enabled,
> if I'm not mistaken; the count just continues increasing from the new
> written value (i.e. no stall/failure). Therefore, there's not need to
> return -EBUSY here and this check can be eliminated.
>
>> +static int ecap_cnt_pol_write(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + size_t idx, enum counter_signal_polarity pol)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + if (ecap_dev->enabled)
>> + return -EBUSY;
> I suspect this check can go away for the same reason as above.
>
>> +static int ecap_cnt_cap_write(struct counter_device *counter,
>> + struct counter_count *count,
>> + size_t idx, u64 cap)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + if (ecap_dev->enabled)
>> + return -EBUSY;
> Same comment as above.
>
>> +static int ecap_cnt_nb_ovf_write(struct counter_device *counter,
>> + struct counter_count *count, u64 val)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + if (ecap_dev->enabled)
>> + return -EBUSY;
> Same comment as above.
>
>> +static struct counter_count ecap_cnt_counts[] = {
>> + {
>> + .id = 0,
> The id member is for differentiating between multiple Counts. You only
> have one Count in this driver so you don't need to set it because you
> never use it.
>
> William Breathitt Gray
Powered by blists - more mailing lists