| 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
| ||
|
Date: Fri, 3 Mar 2023 16:52:53 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: Doug Anderson <dianders@...omium.org>
Cc: Jonathan Corbet <corbet@....net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Abel Vesa <abel.vesa@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Matthias Kaehlcke <mka@...omium.org>, kernel-team@...roid.com,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v1 2/2] driver core: Make state_synced device attribute writeable
On Tue, Feb 28, 2023 at 2:33 PM Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Thu, Feb 23, 2023 at 11:05 PM Saravana Kannan <saravanak@...gle.com> wrote:
> >
> > If the file is written to and sync_state() hasn't been called for the
> > device yet, then call sync_state() for the device independent of the
> > state of its consumers.
> >
> > This is useful for supplier devices that have one or more consumers that
> > don't have a driver but the consumers are in a state that don't use the
> > resources supplied by the supplier device.
> >
> > This gives finer grained control than using the
> > fw_devlink.sync_state=timeout kernel commandline parameter.
> >
> > Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> > ---
> > .../ABI/testing/sysfs-devices-state_synced | 5 +++++
> > drivers/base/base.h | 8 ++++++++
> > drivers/base/core.c | 5 +----
> > drivers/base/dd.c | 18 +++++++++++++++++-
> > 4 files changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
> > index 0c922d7d02fc..cc4090c9df75 100644
> > --- a/Documentation/ABI/testing/sysfs-devices-state_synced
> > +++ b/Documentation/ABI/testing/sysfs-devices-state_synced
> > @@ -21,4 +21,9 @@ Description:
> > at the time the kernel starts are not affected or limited in
> > any way by sync_state() callbacks.
> >
> > + Writing anything to this file will force a call to the device's
> > + sync_state() function if it hasn't been called already. The
> > + sync_state() call happens is independent of the state of the
> > + consumer devices.
>
> Please don't just accept anything written. It doesn't take much to
> check that the user wrote some known value here and then if we ever
> have a reason to allow something else we don't have to break old ABIs.
> Maybe "-1"?
Fine. I'll make it "1" to be consistent with the read behavior.
>
>
> > +
> >
> > diff --git a/drivers/base/base.h b/drivers/base/base.h
> > index 6fcd71803d35..b055eba1ec30 100644
> > --- a/drivers/base/base.h
> > +++ b/drivers/base/base.h
> > @@ -164,6 +164,14 @@ static inline int driver_match_device(struct device_driver *drv,
> > return drv->bus->match ? drv->bus->match(dev, drv) : 1;
> > }
> >
> > +static inline void dev_sync_state(struct device *dev)
>
> IMO don't force inline. The compiler is probably smarter than you. I
> could even believe that it might be more optimal for this rarely
> called function to be _not_ inline if it kept the kernel smaller. I
> guess that means moving it out of the header...
I'm following the style of every other function in the .h file.
>
> > +{
> > + if (dev->bus->sync_state)
> > + dev->bus->sync_state(dev);
> > + else if (dev->driver && dev->driver->sync_state)
> > + dev->driver->sync_state(dev);
> > +}
> > +
> > extern int driver_add_groups(struct device_driver *drv,
> > const struct attribute_group **groups);
> > extern void driver_remove_groups(struct device_driver *drv,
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 929ec218f180..60bb3551977b 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1215,10 +1215,7 @@ static void device_links_flush_sync_list(struct list_head *list,
> > if (dev != dont_lock_dev)
> > device_lock(dev);
> >
> > - if (dev->bus->sync_state)
> > - dev->bus->sync_state(dev);
> > - else if (dev->driver && dev->driver->sync_state)
> > - dev->driver->sync_state(dev);
> > + dev_sync_state(dev);
> >
> > if (dev != dont_lock_dev)
> > device_unlock(dev);
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 84f07e0050dd..17b51573f794 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -510,6 +510,22 @@ EXPORT_SYMBOL_GPL(device_bind_driver);
> > static atomic_t probe_count = ATOMIC_INIT(0);
> > static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
> >
> > +static ssize_t state_synced_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + device_lock(dev);
> > + if (!dev->state_synced) {
> > + dev->state_synced = true;
> > + dev_sync_state(dev);
> > + } else {
> > + count = -EINVAL;
>
> count is of type "size_t", not "ssize_t". -EINVAL is signed.
Heh... I took the time to make sure it was signed... but looks like I
accidentally followed the other typdefs. Also the -EINVAL actually
worked when I tried writing
to a file that already had "1". I guess since the return value is
signed, it worked out. But, I'll fix it.
-Saravana
Powered by blists - more mailing lists