linux-kernel - Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support for ECAP

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yy0G9a5S3OzwyEwW@fedora>
Date:   Thu, 22 Sep 2022 21:08:05 -0400
From:   William Breathitt Gray <william.gray@...aro.org>
To:     Julien Panis <jpanis@...libre.com>
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 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.


> +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, &regval);
> +	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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)