linux-kernel - Re: [PATCH v5 1/1] platform/chrome: add a driver for HPS

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]

Date:   Wed, 12 Oct 2022 21:29:03 -0700
From:   Dan Callaghan <dcallagh@...omium.org>
To:     Tzung-Bi Shih <tzungbi@...nel.org>
Cc:     chrome-platform@...ts.linux.dev,
        LKML <linux-kernel@...r.kernel.org>,
        Sami Kyöstilä <skyostil@...omium.org>,
        Benson Leung <bleung@...omium.org>
Subject: Re: [PATCH v5 1/1] platform/chrome: add a driver for HPS

Excerpts from Tzung-Bi Shih’s message of 2022-10-12 19:46:51 +1100:
> On Wed, Oct 12, 2022 at 03:09:18PM +1100, Dan Callaghan wrote:
> > ---
>
> It doesn't need a cover letter if the series only has 1 patch in general.
> Instead, it could put additional information (and changelogs) after "---".

Understood, I'll omit the cover letter in future postings.

> > diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
> [...]
> > +static int hps_i2c_probe(struct i2c_client *client)
> > +{
> > +     struct hps_drvdata *hps;
> > +     int ret;
> > +
> > +     hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL);
> > +     if (!hps)
> > +             return -ENOMEM;
> > +
> > +     memset(&hps->misc_device, 0, sizeof(hps->misc_device));
>
> The memset can be dropped.  `hps` is z-allocated.

I'll take this out.

> > +     hps->misc_device.parent = &client->dev;
> > +     hps->misc_device.minor = MISC_DYNAMIC_MINOR;
> > +     hps->misc_device.name = "cros-hps";
> > +     hps->misc_device.fops = &hps_fops;
> > +
> > +     i2c_set_clientdata(client, hps);
> > +     hps->client = client;
>
> To be neat, I would prefer to insert a blank line here.

Sure, will add one.

> > +     hps->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(hps->enable_gpio)) {
> > +             ret = PTR_ERR(hps->enable_gpio);
> > +             dev_err(&client->dev, "failed to get enable gpio: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = misc_register(&hps->misc_device);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to initialize misc device: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     hps_set_power(hps, false);
>
> IIUC, the GPIO will raise to HIGH in the first place, and then fall
> to LOW until here.  Is it an expected behavior?  How about gpiod_get()
> with GPIOD_OUT_LOW?

It might seem a little unusual, but it is intentional. The enable line is
already high when we enter the kernel from firmware. Acquiring the GPIO
line with GPIOD_OUT_HIGH preserves its existing state (high) in case later
steps fail.

We power off the periphal only once the driver is successfully bound and has
taken control of its power state.

> > +static int hps_i2c_remove(struct i2c_client *client)
> > +{
> > +     struct hps_drvdata *hps = i2c_get_clientdata(client);
> > +
> > +     pm_runtime_disable(&client->dev);
> > +     misc_deregister(&hps->misc_device);
> > +     hps_set_power(hps, true);
>
> Why does it need to raise the GPIO again when removing the device?

Similar to the above, we want to preserve the default power state
(i.e. powered on) whenever the driver is not bound to the device.

This behaviour made sense to us mainly because we were originally controlling
the peripheral entirely from userspace, so it was always powered on by default.

Do you think this behaviour is acceptable, or do we need to change it?

-- 
Dan Callaghan <dcallagh@...omium.org>
Software Engineer, Google