Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190704084617.3602-1-gregkh@linuxfoundation.org>
 <20190704084617.3602-2-gregkh@linuxfoundation.org> <20190704093200.GM13424@localhost>
 <20190704104311.GA16681@kroah.com> <20190704121143.GA5007@kroah.com>
In-Reply-To: <20190704121143.GA5007@kroah.com>
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date:   Thu, 4 Jul 2019 14:17:22 -0700
Message-ID: <CAKdAkRQ4W7wjYjZcBn4_s+PD26pv_8mrjUt-ne24GkimGEXoiA@mail.gmail.com>
Subject: Re: [PATCH 01/12 v2] Platform: add a dev_groups pointer to struct platform_driver
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Johan Hovold <johan@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Richard Gong <richard.gong@linux.intel.com>,
        Romain Izard <romain.izard.pro@gmail.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Mans Rullgard <mans@mansr.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Randy Dunlap <rdunlap@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Greg,

On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Platform drivers like to add sysfs groups to their device, but right now
> they have to do it "by hand".  The driver core should handle this for
> them, but there is no way to get to the bus-default attribute groups as
> all platform devices are "special and unique" one-off drivers/devices.
>
> To combat this, add a dev_groups pointer to platform_driver which allows
> a platform driver to set up a list of default attributes that will be
> properly created and removed by the platform driver core when a probe()
> function is successful and removed right before the device is unbound.

Why is this limited to platform bus? Drivers for other buses also
often want to augment list of their attributes during probe(). I'd
move it to generic probe handling.

>
> Cc: Richard Gong <richard.gong@linux.intel.com>
> Cc: Romain Izard <romain.izard.pro@gmail.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mans Rullgard <mans@mansr.com>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2: addressed Johan's comments by reordering when we remove the files
>     from the device, and clean up on an error in a nicer way.  Ended up
>     making the patch smaller overall, always nice.
>
>  drivers/base/platform.c         | 16 +++++++++++++++-
>  include/linux/platform_device.h |  1 +
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 713903290385..74428a1e03f3 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -614,8 +614,20 @@ static int platform_drv_probe(struct device *_dev)
>
>         if (drv->probe) {
>                 ret = drv->probe(dev);
> -               if (ret)
> +               if (ret) {
> +                       dev_pm_domain_detach(_dev, true);
> +                       goto out;
> +               }
> +       }
> +       if (drv->dev_groups) {
> +               ret = device_add_groups(_dev, drv->dev_groups);
> +               if (ret) {
> +                       if (drv->remove)
> +                               drv->remove(dev);
>                         dev_pm_domain_detach(_dev, true);
> +                       return ret;
> +               }
> +               kobject_uevent(&_dev->kobj, KOBJ_CHANGE);

We already emit KOBJ_BIND when we finish binding device to a driver,
regardless of the bus. I know we still need to teach systemd to handle
it properly, but I think it is better than sprinkling KOBJ_CHANGE
around.

Thanks.

-- 
Dmitry