| 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: <c03fd83f-97b3-1837-b7fd-5f3f28c4b557@gmail.com>
Date: Tue, 12 Jul 2022 10:30:41 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Naresh Solanki <naresh.solanki@...ements.com>,
linux-kernel@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>
Cc: Patrick Rudolph <patrick.rudolph@...ements.com>,
Marcello Sylvester Bauer <sylv@...v.io>
Subject: Re: [PATCH 3/5] regulator: max597x: Add support for max597x regulator
On 7/5/22 15:22, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@...ements.com>
>
> max597x is hot swap controller.
> This regulator driver controls the same & also configures fault
> protection features supported by the chip.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
> Signed-off-by: Marcello Sylvester Bauer <sylv@...v.io>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.com>
I like the way the IRQ helpers have been used here. It'd be cool to hear
how the rest of the system you're dealing with utilize the WARN level
events :)
> +static int max597x_set_ocp(struct regulator_dev *rdev, int lim_uA,
> + int severity, bool enable)
> +{
> + int ret, val, reg;
> + unsigned int vthst, vthfst;
> +
> + struct max597x_regulator *data = rdev_get_drvdata(rdev);
> + int rdev_id = rdev_get_id(rdev);
> + /*
> + * MAX5970 doesn't has enable control for ocp.
> + * If limit is specified but enable is not set then hold the value in
> + * variable & later use it when ocp needs to be enabled.
> + */
Is this a possible scenario? I think that if a non zero limit is given
in a "regulator-oc-protection-microamp"-property, then the protection
should always be enabled. Am I overlooking something?
> + if (lim_uA != 0 && lim_uA != data->lim_uA)
> + data->lim_uA = lim_uA;
> +
> + if (severity != REGULATOR_SEVERITY_PROT)
> + return -EINVAL;
> +
> + if (enable) {
> +
> + /* Calc Vtrip threshold in uV. */
> + vthst =
> + div_u64(mul_u32_u32(data->shunt_micro_ohms, data->lim_uA),
> + 1000000);
> +
> + /*
> + * As recommended in datasheed, add 20% margin to avoid
> + * spurious event & passive component tolerance.
> + */
> + vthst = div_u64(mul_u32_u32(vthst, 120), 100);
> +
> + /* Calc fast Vtrip threshold in uV */
> + vthfst = vthst * (MAX5970_FAST2SLOW_RATIO / 100);
> +
> + if (vthfst > data->irng) {
> + dev_err(&rdev->dev, "Current limit out of range\n");
> + return -EINVAL;
> + }
> + /* Fast trip threshold to be programmed */
> + val = div_u64(mul_u32_u32(0xFF, vthfst), data->irng);
> + } else
> + /*
> + * Since there is no option to disable ocp, set limit to max
> + * value
> + */
> + val = 0xFF;
> +
> + reg = MAX5970_REG_DAC_FAST(rdev_id);
> + ret = regmap_write(rdev->regmap, reg, val);
> +
> + return ret;
> +}
> +
> +static int max597x_irq_handler(int irq, struct regulator_irq_data *rid,
> + unsigned long *dev_mask)
> +{
> + struct regulator_err_state *stat;
> + struct max597x_regulator *d = (struct max597x_regulator *)rid->data;
> + int val, ret, i;
> + > + ret = max597x_regmap_read_clear(d->regmap, MAX5970_REG_FAULT0, &val);
> + if (ret)
> + return REGULATOR_FAILED_RETRY;
This "read_clear" smells like a race-by-design to me...
> +
> + *dev_mask = 0;
> + for (i = 0; i < d->num_switches; i++) {
> + stat = &rid->states[i];
> + stat->notifs = 0;
> + stat->errors = 0;
> + }
> +
> + for (i = 0; i < d->num_switches; i++) {
> + stat = &rid->states[i];
> +
> + if (val & UV_STATUS_CRIT(i)) {
> + *dev_mask |= 1 << i;
> + stat->notifs |= REGULATOR_EVENT_UNDER_VOLTAGE;
> + stat->errors |= REGULATOR_ERROR_UNDER_VOLTAGE;
> + } else if (val & UV_STATUS_WARN(i)) {
> + *dev_mask |= 1 << i;
> + stat->notifs |= REGULATOR_EVENT_UNDER_VOLTAGE_WARN;
> + stat->errors |= REGULATOR_ERROR_UNDER_VOLTAGE_WARN;
> + }
> + }
> +
> + ret = max597x_regmap_read_clear(d->regmap, MAX5970_REG_FAULT1, &val);
> + if (ret)
> + return REGULATOR_FAILED_RETRY;
... and same here...
> +
> + for (i = 0; i < d->num_switches; i++) {
> + stat = &rid->states[i];
> +
> + if (val & OV_STATUS_CRIT(i)) {
> + *dev_mask |= 1 << i;
> + stat->notifs |= REGULATOR_EVENT_REGULATION_OUT;
> + stat->errors |= REGULATOR_ERROR_REGULATION_OUT;
> + } else if (val & OV_STATUS_WARN(i)) {
> + *dev_mask |= 1 << i;
> + stat->notifs |= REGULATOR_EVENT_OVER_VOLTAGE_WARN;
> + stat->errors |= REGULATOR_ERROR_OVER_VOLTAGE_WARN;
> + }
> + }
> +
> + ret = max597x_regmap_read_clear(d->regmap, MAX5970_REG_FAULT2, &val);
> + if (ret)
> + return REGULATOR_FAILED_RETRY;
> +
... and here. I wonder if the reason for "clearing" would be worth
commenting?
> + for (i = 0; i < d->num_switches; i++) {
> + stat = &rid->states[i];
> +
> + if (val & OC_STATUS_WARN(i)) {
> + *dev_mask |= 1 << i;
> + stat->notifs |= REGULATOR_EVENT_OVER_CURRENT_WARN;
> + stat->errors |= REGULATOR_ERROR_OVER_CURRENT_WARN;
> + }
> + }
> +
> + ret = regmap_read(d->regmap, MAX5970_REG_STATUS0, &val);
> + if (ret)
> + return REGULATOR_FAILED_RETRY;
> +
> + for (i = 0; i < d->num_switches; i++) {
> + stat = &rid->states[i];
> +
> + if ((val & MAX5970_CB_IFAULTF(i))
> + || (val & MAX5970_CB_IFAULTS(i))) {
> + *dev_mask |= 1 << i;
> + stat->notifs |=
> + REGULATOR_EVENT_OVER_CURRENT |
> + REGULATOR_EVENT_DISABLE;
> + stat->errors |=
> + REGULATOR_ERROR_OVER_CURRENT | REGULATOR_ERROR_FAIL;
> +
> + /* Clear the sub-IRQ status */
> + regulator_disable_regmap(stat->rdev);
> + }
> + }
> + return 0;
> +}
> +
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));
Powered by blists - more mailing lists