linux-kernel - Re: [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support

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: <20220412193013.GA25279@jagath-PC>
Date:   Wed, 13 Apr 2022 01:00:15 +0530
From:   Jagath Jog J <jagathjog1996@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Dan Robertson <dan@...obertson.com>,
        Jonathan Cameron <jic23@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support

Hello Andy,

On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <jagathjog1996@...il.com> wrote:
> >
> > Added trigger buffer support to read continuous acceleration
> > data from device with data ready interrupt which is mapped
> > to INT1 pin.
> 
> Can you explain the locking schema in this driver?
> 
> > +       /* Configure INT1 pin to open drain */
> > +       ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> > +       if (ret)
> > +               return ret;
> 
> No locking (or regmap only locking).

This is bma400_init() function which will run when probe runs so there is no
locking in this bma400_init().

> 
> ...
> 
> > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > +                                            bool state)
> > +{
> > +       struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +       struct bma400_data *data = iio_priv(indio_dev);
> > +       int ret;
> > +
> > +       ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> > +                                BMA400_INT_DRDY_MSK,
> > +                                FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> > +       if (ret)
> > +               return ret;
> > +
> > +       return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> > +                                 BMA400_INT_DRDY_MSK,
> > +                                 FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> > +}
> 
> Ditto.

Sorry, I missed this.
I will add lock and unlocking in the next patch.

> 
> ...
> 
> > +       mutex_lock(&data->mutex);
> > +
> > +       /* bulk read six registers, with the base being the LSB register */
> > +       ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> > +                              &data->buffer.buff, sizeof(data->buffer.buff));
> > +       mutex_unlock(&data->mutex);
> > +       if (ret)
> > +               return IRQ_NONE;
> 
> But here only above with locking...
> 
> > +       ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> > +       if (ret)
> > +               return IRQ_NONE;
> 
> ...followed by no locking.

Okay I will add lock in the next patch.

> 
> ...
> 
> > +       mutex_lock(&data->mutex);
> > +       ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> > +                              sizeof(status));
> > +       mutex_unlock(&data->mutex);
> > +       if (ret)
> > +               return IRQ_NONE;
> 
> And again with locking.
> 
> ...
> 
> So,
> 1) Does regmap is configured with locking? What for?
> 2) What's the role of data->mutex?

1.
NO, regmap is not configured with locking. 
In the regmap_config structure variable below these members are not defined
in the driver.

struct regmap_config {
	regmap_lock lock;
	regmap_unlock unlock;
	void *lock_arg;

2.
data->mutex is used to protect the register read, write access from the device.

Is the regmap functions handle locking and unlocking internally if these below
struct members are not defined?

regmap_lock lock;
regmap_unlock unlock;
void *lock_arg;


> 
> -- 
> With Best Regards,
> Andy Shevchenko