| Message ID | b991506bcbf665f7af185945f70bf9d5cf04637c.1645804976.git.sylv@sylv.io (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1,1/1] drivers/hwmon/pmbus: Add mutex to regulator ops | expand |
On Fri, Feb 25, 2022 at 08:06:09AM PST, Marcello Sylvester Bauer wrote: >From: Patrick Rudolph <patrick.rudolph@9elements.com> > >On PMBUS devices with multiple pages, the regulator ops need to be >protected with the update mutex. This prevents accidentally changing >the page in a separate thread while operating on the PMBUS_OPERATION >register. > >Tested on Infineon xdpe11280 while a separate thread polls for sensor >data. > >Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >--- > drivers/hwmon/pmbus/pmbus_core.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > >diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >index 776ee2237be2..f79930162256 100644 >--- a/drivers/hwmon/pmbus/pmbus_core.c >+++ b/drivers/hwmon/pmbus/pmbus_core.c >@@ -2386,10 +2386,14 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev) > { > struct device *dev = rdev_get_dev(rdev); > struct i2c_client *client = to_i2c_client(dev->parent); >+ struct pmbus_data *data = i2c_get_clientdata(client); > u8 page = rdev_get_id(rdev); > int ret; > >+ mutex_lock(&data->update_lock); > ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION); >+ mutex_unlock(&data->update_lock); >+ > if (ret < 0) > return ret; > >@@ -2400,11 +2404,17 @@ static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable) > { > struct device *dev = rdev_get_dev(rdev); > struct i2c_client *client = to_i2c_client(dev->parent); >+ struct pmbus_data *data = i2c_get_clientdata(client); > u8 page = rdev_get_id(rdev); >+ int ret; > >- return pmbus_update_byte_data(client, page, PMBUS_OPERATION, >- PB_OPERATION_CONTROL_ON, >- enable ? PB_OPERATION_CONTROL_ON : 0); >+ mutex_lock(&data->update_lock); >+ ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION, >+ PB_OPERATION_CONTROL_ON, >+ enable ? PB_OPERATION_CONTROL_ON : 0); >+ mutex_unlock(&data->update_lock); >+ >+ return ret; > } > > static int pmbus_regulator_enable(struct regulator_dev *rdev) >-- >2.35.1 > > Looks like this doesn't cover pmbus_regulator_get_error_flags(), which was added recently -- perhaps rebase onto hwmon-next? Zev
On 2/26/22 15:42, Zev Weiss wrote: > On Fri, Feb 25, 2022 at 08:06:09AM PST, Marcello Sylvester Bauer wrote: >> From: Patrick Rudolph <patrick.rudolph@9elements.com> >> >> On PMBUS devices with multiple pages, the regulator ops need to be >> protected with the update mutex. This prevents accidentally changing >> the page in a separate thread while operating on the PMBUS_OPERATION >> register. >> >> Tested on Infineon xdpe11280 while a separate thread polls for sensor >> data. >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >> --- >> drivers/hwmon/pmbus/pmbus_core.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 776ee2237be2..f79930162256 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -2386,10 +2386,14 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev) >> { >> struct device *dev = rdev_get_dev(rdev); >> struct i2c_client *client = to_i2c_client(dev->parent); >> + struct pmbus_data *data = i2c_get_clientdata(client); >> u8 page = rdev_get_id(rdev); >> int ret; >> >> + mutex_lock(&data->update_lock); >> ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION); >> + mutex_unlock(&data->update_lock); >> + >> if (ret < 0) >> return ret; >> >> @@ -2400,11 +2404,17 @@ static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable) >> { >> struct device *dev = rdev_get_dev(rdev); >> struct i2c_client *client = to_i2c_client(dev->parent); >> + struct pmbus_data *data = i2c_get_clientdata(client); >> u8 page = rdev_get_id(rdev); >> + int ret; >> >> - return pmbus_update_byte_data(client, page, PMBUS_OPERATION, >> - PB_OPERATION_CONTROL_ON, >> - enable ? PB_OPERATION_CONTROL_ON : 0); >> + mutex_lock(&data->update_lock); >> + ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION, >> + PB_OPERATION_CONTROL_ON, >> + enable ? PB_OPERATION_CONTROL_ON : 0); >> + mutex_unlock(&data->update_lock); >> + >> + return ret; >> } >> >> static int pmbus_regulator_enable(struct regulator_dev *rdev) >> -- >> 2.35.1 >> >> > > Looks like this doesn't cover pmbus_regulator_get_error_flags(), which > was added recently -- perhaps rebase onto hwmon-next? > This is a bug fix which needs to be applied to stable releases, or am I missing something ? Guenter
On Sat, Feb 26, 2022 at 03:47:41PM PST, Guenter Roeck wrote: >On 2/26/22 15:42, Zev Weiss wrote: >>On Fri, Feb 25, 2022 at 08:06:09AM PST, Marcello Sylvester Bauer wrote: >>>From: Patrick Rudolph <patrick.rudolph@9elements.com> >>> >>>On PMBUS devices with multiple pages, the regulator ops need to be >>>protected with the update mutex. This prevents accidentally changing >>>the page in a separate thread while operating on the PMBUS_OPERATION >>>register. >>> >>>Tested on Infineon xdpe11280 while a separate thread polls for sensor >>>data. >>> >>>Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >>>Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >>>--- >>>drivers/hwmon/pmbus/pmbus_core.c | 16 +++++++++++++--- >>>1 file changed, 13 insertions(+), 3 deletions(-) >>> >>>diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >>>index 776ee2237be2..f79930162256 100644 >>>--- a/drivers/hwmon/pmbus/pmbus_core.c >>>+++ b/drivers/hwmon/pmbus/pmbus_core.c >>>@@ -2386,10 +2386,14 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev) >>>{ >>> struct device *dev = rdev_get_dev(rdev); >>> struct i2c_client *client = to_i2c_client(dev->parent); >>>+ struct pmbus_data *data = i2c_get_clientdata(client); >>> u8 page = rdev_get_id(rdev); >>> int ret; >>> >>>+ mutex_lock(&data->update_lock); >>> ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION); >>>+ mutex_unlock(&data->update_lock); >>>+ >>> if (ret < 0) >>> return ret; >>> >>>@@ -2400,11 +2404,17 @@ static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable) >>>{ >>> struct device *dev = rdev_get_dev(rdev); >>> struct i2c_client *client = to_i2c_client(dev->parent); >>>+ struct pmbus_data *data = i2c_get_clientdata(client); >>> u8 page = rdev_get_id(rdev); >>>+ int ret; >>> >>>- return pmbus_update_byte_data(client, page, PMBUS_OPERATION, >>>- PB_OPERATION_CONTROL_ON, >>>- enable ? PB_OPERATION_CONTROL_ON : 0); >>>+ mutex_lock(&data->update_lock); >>>+ ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION, >>>+ PB_OPERATION_CONTROL_ON, >>>+ enable ? PB_OPERATION_CONTROL_ON : 0); >>>+ mutex_unlock(&data->update_lock); >>>+ >>>+ return ret; >>>} >>> >>>static int pmbus_regulator_enable(struct regulator_dev *rdev) >>>-- >>>2.35.1 >>> >>> >> >>Looks like this doesn't cover pmbus_regulator_get_error_flags(), which >>was added recently -- perhaps rebase onto hwmon-next? >> > >This is a bug fix which needs to be applied to stable releases, or am I >missing something ? > Ah, true -- should it then be a two-patch series, with this as the first for -stable and then a second for -next that extends it to include pmbus_regulator_get_error_flags()? Zev
On Fri, Feb 25, 2022 at 05:06:09PM +0100, Marcello Sylvester Bauer wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > On PMBUS devices with multiple pages, the regulator ops need to be > protected with the update mutex. This prevents accidentally changing > the page in a separate thread while operating on the PMBUS_OPERATION > register. > > Tested on Infineon xdpe11280 while a separate thread polls for sensor > data. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> Good catch. Applied. Thanks, Guenter > --- > drivers/hwmon/pmbus/pmbus_core.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 776ee2237be2..f79930162256 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2386,10 +2386,14 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev) > { > struct device *dev = rdev_get_dev(rdev); > struct i2c_client *client = to_i2c_client(dev->parent); > + struct pmbus_data *data = i2c_get_clientdata(client); > u8 page = rdev_get_id(rdev); > int ret; > > + mutex_lock(&data->update_lock); > ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION); > + mutex_unlock(&data->update_lock); > + > if (ret < 0) > return ret; > > @@ -2400,11 +2404,17 @@ static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable) > { > struct device *dev = rdev_get_dev(rdev); > struct i2c_client *client = to_i2c_client(dev->parent); > + struct pmbus_data *data = i2c_get_clientdata(client); > u8 page = rdev_get_id(rdev); > + int ret; > > - return pmbus_update_byte_data(client, page, PMBUS_OPERATION, > - PB_OPERATION_CONTROL_ON, > - enable ? PB_OPERATION_CONTROL_ON : 0); > + mutex_lock(&data->update_lock); > + ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION, > + PB_OPERATION_CONTROL_ON, > + enable ? PB_OPERATION_CONTROL_ON : 0); > + mutex_unlock(&data->update_lock); > + > + return ret; > } > > static int pmbus_regulator_enable(struct regulator_dev *rdev)
On 2/26/22 15:42, Zev Weiss wrote: > On Fri, Feb 25, 2022 at 08:06:09AM PST, Marcello Sylvester Bauer wrote: >> From: Patrick Rudolph <patrick.rudolph@9elements.com> >> >> On PMBUS devices with multiple pages, the regulator ops need to be >> protected with the update mutex. This prevents accidentally changing >> the page in a separate thread while operating on the PMBUS_OPERATION >> register. >> >> Tested on Infineon xdpe11280 while a separate thread polls for sensor >> data. >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >> --- >> drivers/hwmon/pmbus/pmbus_core.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 776ee2237be2..f79930162256 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -2386,10 +2386,14 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev) >> { >> struct device *dev = rdev_get_dev(rdev); >> struct i2c_client *client = to_i2c_client(dev->parent); >> + struct pmbus_data *data = i2c_get_clientdata(client); >> u8 page = rdev_get_id(rdev); >> int ret; >> >> + mutex_lock(&data->update_lock); >> ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION); >> + mutex_unlock(&data->update_lock); >> + >> if (ret < 0) >> return ret; >> >> @@ -2400,11 +2404,17 @@ static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable) >> { >> struct device *dev = rdev_get_dev(rdev); >> struct i2c_client *client = to_i2c_client(dev->parent); >> + struct pmbus_data *data = i2c_get_clientdata(client); >> u8 page = rdev_get_id(rdev); >> + int ret; >> >> - return pmbus_update_byte_data(client, page, PMBUS_OPERATION, >> - PB_OPERATION_CONTROL_ON, >> - enable ? PB_OPERATION_CONTROL_ON : 0); >> + mutex_lock(&data->update_lock); >> + ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION, >> + PB_OPERATION_CONTROL_ON, >> + enable ? PB_OPERATION_CONTROL_ON : 0); >> + mutex_unlock(&data->update_lock); >> + >> + return ret; >> } >> >> static int pmbus_regulator_enable(struct regulator_dev *rdev) >> -- >> 2.35.1 >> >> > > Looks like this doesn't cover pmbus_regulator_get_error_flags(), which > was added recently -- perhaps rebase onto hwmon-next? > I applied the patch and made the necessary changes in pmbus_regulator_get_error_flags(), so no need to send a patch for it. Guenter
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 776ee2237be2..f79930162256 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2386,10 +2386,14 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev) { struct device *dev = rdev_get_dev(rdev); struct i2c_client *client = to_i2c_client(dev->parent); + struct pmbus_data *data = i2c_get_clientdata(client); u8 page = rdev_get_id(rdev); int ret; + mutex_lock(&data->update_lock); ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION); + mutex_unlock(&data->update_lock); + if (ret < 0) return ret; @@ -2400,11 +2404,17 @@ static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable) { struct device *dev = rdev_get_dev(rdev); struct i2c_client *client = to_i2c_client(dev->parent); + struct pmbus_data *data = i2c_get_clientdata(client); u8 page = rdev_get_id(rdev); + int ret; - return pmbus_update_byte_data(client, page, PMBUS_OPERATION, - PB_OPERATION_CONTROL_ON, - enable ? PB_OPERATION_CONTROL_ON : 0); + mutex_lock(&data->update_lock); + ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION, + PB_OPERATION_CONTROL_ON, + enable ? PB_OPERATION_CONTROL_ON : 0); + mutex_unlock(&data->update_lock); + + return ret; } static int pmbus_regulator_enable(struct regulator_dev *rdev)