linux-kernel - Re: [PATCHv2] regulator: s2mps11: Added shutdown function to poweroff

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] [day] [month] [year] [list]
Date:	Wed, 15 Jul 2015 08:12:07 +0530
From:	Anand Moon <linux.amoon@...il.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
Cc:	Sangbeom Kim <sbkim73@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>
Subject: Re: [PATCHv2] regulator: s2mps11: Added shutdown function to poweroff

hi Krzysztof,

On 15 July 2015 at 06:33, Krzysztof Kozlowski <k.kozlowski@...sung.com> wrote:
> On 15.07.2015 00:41, Anand Moon wrote:
>> Added .shutdown function to s2mps11 to help poweroff the board successfully.
>
> Which board does not poweroff? The PMIC is used on multiple boards, did
> you observed this on all of them?
>
>>
>> s2mps11-pmic: S2MPS11_REG_CTRL1 reg value 16:00000000000000000000000000010000
>>
>> The device driver clears the register to turn off the PMIC.
>
> This is not sufficient explanation for commit message.
>
> I already raised concerns that this does not look to me as a proper way
> of doing poweroff. Unfortunately you did not resolved these concerns.
>
> The main questions are unanswered: Why you have to do this and why
> "standard" way does not work?
> How can you properly fix some problem if you don't know the cause of
> problem? It is blind shooting which may hurt other boards.
>
>
>>
>> Signed-off-by: Anand Moon <linux.amoon@...il.com>
>>
>> ---
>> Console log for improper shutdown.
>> root@...oidxu3:~# poweroff
>> ...
>>   * Unmounting temporary filesystems...                                   [ OK ]
>>   * Deactivating swap...                                                  [ OK ]
>>   * Unmounting local filesystems...                                       [ OK ]
>>   * Will now halt
>>   [  209.020280] reboot: Power down
>>   [  209.122039] Power down failed, please power off system manually.
>>
>> Console log for proper shutdown.
>> root@...oidxu3:~# poweroff
>> ...
>>   * Unmounting temporary filesystems...                                   [ OK ]
>>   * Deactivating swap...                                                  [ OK ]
>>   * Unmounting local filesystems...                                       [ OK ]
>>   * Will now halt
>>   [  476.283071] reboo
>> ---
>>  drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
>> index 326ffb5..823180e 100644
>> --- a/drivers/regulator/s2mps11.c
>> +++ b/drivers/regulator/s2mps11.c
>> @@ -1060,6 +1060,31 @@ out:
>>       return ret;
>>  }
>>
>> +static void s2mps11_pmic_shutdown(struct platform_device *pdev)
>> +{
>> +     struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>> +     unsigned int reg_val, ret;
>> +
>> +     ret = regmap_read(iodev->regmap_pmic, S2MPS11_REG_CTRL1, &reg_val);
>> +     if (ret < 0) {
>
> regmap_read() returns an int which you assign to an unsigned int which
> then you compare against <0? This does not look good.
>
>> +             dev_crit(&pdev->dev, "could not read S2MPS11_REG_CTRL1 value\n");
>> +     } else {
>> +             /*
>> +              * s2mps11-pmic: S2MPS11_REG_CTRL1 reg value
>> +              * is 00000000000000000000000000010000
>> +              * clear the S2MPS11_REG_CTRL1 0x10 value to shutdown.
>> +              */
>> +             if (reg_val & BIT(4)) {
>> +                     ret = regmap_update_bits(iodev->regmap_pmic,
>> +                                              S2MPS11_REG_CTRL1,
>> +                                              BIT(4), BIT(0));
>
> I don't understand. You want to update BIT(4) but the value is BIT(0)?
> This will clear BIT(4) but is totally unreadable.
>
>> +                     if (ret)
>> +                             dev_crit(&pdev->dev,
>> +                                      "could not write S2MPS11_REG_CTRL1 value\n");
>> +             }
>> +     }
>
> The code is not readable, to many unnecessary indentations.
>
>> +}
>> +
>>  static const struct platform_device_id s2mps11_pmic_id[] = {
>>       { "s2mps11-pmic", S2MPS11X},
>>       { "s2mps13-pmic", S2MPS13X},
>> @@ -1074,6 +1099,7 @@ static struct platform_driver s2mps11_pmic_driver = {
>>               .name = "s2mps11-pmic",
>>       },
>>       .probe = s2mps11_pmic_probe,
>> +     .shutdown = s2mps11_pmic_shutdown,
>
> The purpose of shutdown function is not to shutdown the system but to
> prepare the system for shutdown.
>
> The patch is just wrong and you did not answered the major question -
> WHY you have to do this? Don't fix the problem blindly (or because some
> hardkernel tree for some of the boards use such patch).
>
> Best regards,
> Krzysztof
>
>>       .id_table = s2mps11_pmic_id,
>>  };
>>
>>
>

I might me missing the bigger picture. So drop it.

-Anand Moon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/