[PATCH for-4.16 0/3] drivers: base: revise coredump ABI

[PATCH for-4.16 0/3] drivers: base: revise coredump ABI

[Arend van Spriel]

Not sure if this can be considered as fix for v4.16. Upon submitting
the driver patches I received valuable feedback that I would like to
address with this series.

The patch apply to master and driver-core-next of the driver-core
repository so you can take it either way.

Arend van Spriel (3):
  sysfs: improve devices-coredump description with user-space
    perspective
  drivers: change struct device_driver::coredump() return type to void
  drivers: base: remove check for callback in coredump_store()

 Documentation/ABI/testing/sysfs-devices-coredump | 14 +++++++++-----
 drivers/base/dd.c                                |  3 +--
 include/linux/device.h                           |  5 ++++-
 3 files changed, 14 insertions(+), 8 deletions(-)

--
1.9.1

[PATCH for-4.16 1/3] sysfs: improve devices-coredump description with user-space perspective

[Arend van Spriel]

Instead of referring to kernel internals, describe the ABI from user-space
perspective to clarify what can be expected when using it.

Signed-off-by: Arend van Spriel <aspriel@gmail.com>
---
 Documentation/ABI/testing/sysfs-devices-coredump | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-coredump b/Documentation/ABI/testing/sysfs-devices-coredump
index e459368..d5a4c75 100644
--- a/Documentation/ABI/testing/sysfs-devices-coredump
+++ b/Documentation/ABI/testing/sysfs-devices-coredump
@@ -2,9 +2,13 @@ What:		/sys/devices/.../coredump
 Date:		December 2017
 Contact:	Arend van Spriel <aspriel@gmail.com>
 Description:
-		The /sys/devices/.../coredump attribute is only present when the
-		device is bound to a driver, which provides the .coredump()
-		callback. The attribute is write only. Anything written to this
-		file will trigger the .coredump() callback.
+		When present the /sys/devices/.../coredump attribute can be used
+		to trigger a coredump of the device. The coredump contents are
+		device driver specific and thus vary. The coredump attribute is
+		writeonly. Anything written to this file will trigger creation
+		of the coredump. When the coredump is made available under
+		/sys/class/devcoredump it will generate a uevent. When the
+		coredump can not be successfully generated no ueven will occur.

-		Available when CONFIG_DEV_COREDUMP is enabled.
+		Available when CONFIG_DEV_COREDUMP is enabled and the device
+		driver supports coredump generation.
--
1.9.1

Re: [PATCH for-4.16 1/3] sysfs: improve devices-coredump description with user-space perspective

[Brian Norris]

On Thu, Mar 15, 2018 at 10:55:23AM +0100, Arend van Spriel wrote:
> Instead of referring to kernel internals, describe the ABI from user-space
> perspective to clarify what can be expected when using it.
> 
> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-devices-coredump | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-coredump b/Documentation/ABI/testing/sysfs-devices-coredump
> index e459368..d5a4c75 100644
> --- a/Documentation/ABI/testing/sysfs-devices-coredump
> +++ b/Documentation/ABI/testing/sysfs-devices-coredump
> @@ -2,9 +2,13 @@ What:		/sys/devices/.../coredump
>  Date:		December 2017
>  Contact:	Arend van Spriel <aspriel@gmail.com>
>  Description:
> -		The /sys/devices/.../coredump attribute is only present when the
> -		device is bound to a driver, which provides the .coredump()
> -		callback. The attribute is write only. Anything written to this
> -		file will trigger the .coredump() callback.
> +		When present the /sys/devices/.../coredump attribute can be used
> +		to trigger a coredump of the device. The coredump contents are
> +		device driver specific and thus vary. The coredump attribute is
> +		writeonly. Anything written to this file will trigger creation
> +		of the coredump. When the coredump is made available under
> +		/sys/class/devcoredump it will generate a uevent. When the
> +		coredump can not be successfully generated no ueven will occur.

s/ueven/uevent/

> 
> -		Available when CONFIG_DEV_COREDUMP is enabled.
> +		Available when CONFIG_DEV_COREDUMP is enabled and the device
> +		driver supports coredump generation.

What about /sys/class/devcoredump/disabled? Maybe we just need a
sysfs-class-devcoredump too, now that there's a formal method for
triggering devcoredumps.

Brian

> --
> 1.9.1
> 

Re: [PATCH for-4.16 1/3] sysfs: improve devices-coredump description with user-space perspective

[Arend van Spriel]

On 3/15/2018 6:05 PM, Brian Norris wrote:
> On Thu, Mar 15, 2018 at 10:55:23AM +0100, Arend van Spriel wrote:
>> Instead of referring to kernel internals, describe the ABI from user-space
>> perspective to clarify what can be expected when using it.
>>
>> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
>> ---
>>   Documentation/ABI/testing/sysfs-devices-coredump | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-devices-coredump b/Documentation/ABI/testing/sysfs-devices-coredump
>> index e459368..d5a4c75 100644
>> --- a/Documentation/ABI/testing/sysfs-devices-coredump
>> +++ b/Documentation/ABI/testing/sysfs-devices-coredump
>> @@ -2,9 +2,13 @@ What:		/sys/devices/.../coredump
>>   Date:		December 2017
>>   Contact:	Arend van Spriel <aspriel@gmail.com>
>>   Description:
>> -		The /sys/devices/.../coredump attribute is only present when the
>> -		device is bound to a driver, which provides the .coredump()
>> -		callback. The attribute is write only. Anything written to this
>> -		file will trigger the .coredump() callback.
>> +		When present the /sys/devices/.../coredump attribute can be used
>> +		to trigger a coredump of the device. The coredump contents are
>> +		device driver specific and thus vary. The coredump attribute is
>> +		writeonly. Anything written to this file will trigger creation
>> +		of the coredump. When the coredump is made available under
>> +		/sys/class/devcoredump it will generate a uevent. When the
>> +		coredump can not be successfully generated no ueven will occur.
>
> s/ueven/uevent/

crap. I mean: Thanks ;-)

>>
>> -		Available when CONFIG_DEV_COREDUMP is enabled.
>> +		Available when CONFIG_DEV_COREDUMP is enabled and the device
>> +		driver supports coredump generation.
>
> What about /sys/class/devcoredump/disabled? Maybe we just need a
> sysfs-class-devcoredump too, now that there's a formal method for
> triggering devcoredumps.

I am not sure about this. Not my daily routine so I can use some 
opinions here. Not that yours does not count, but I would like to be 
sure before respinning.

Regards,
Arend

[PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Arend van Spriel]

Upon submitting a patch for mwifiex [1] it was discussed whether this
callback function could fail. To keep things simple there is no need
for the error code so the driver can do the task synchronous or not
without worries. Currently the device driver core already ignores the
return value so changing it to void.

[1] https://patchwork.kernel.org/patch/10231933/

Signed-off-by: Arend van Spriel <aspriel@gmail.com>
---
 include/linux/device.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index b093405..f08c25b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -256,6 +256,9 @@ enum probe_type {
  *		automatically.
  * @pm:		Power management operations of the device which matched
  *		this driver.
+ * @coredump:	Called when sysfs entry is written to. The device driver
+ *		is expected to call the dev_coredump API resulting in a
+ *		uevent.
  * @p:		Driver core's private data, no one other than the driver
  *		core can touch this.
  *
@@ -287,7 +290,7 @@ struct device_driver {
 	const struct attribute_group **groups;

 	const struct dev_pm_ops *pm;
-	int (*coredump) (struct device *dev);
+	void (*coredump) (struct device *dev);

 	struct driver_private *p;
 };
--
1.9.1

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Greg Kroah-Hartman]

On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
> Upon submitting a patch for mwifiex [1] it was discussed whether this
> callback function could fail. To keep things simple there is no need
> for the error code so the driver can do the task synchronous or not
> without worries. Currently the device driver core already ignores the
> return value so changing it to void.
> 
> [1] https://patchwork.kernel.org/patch/10231933/
> 
> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
> ---
>  include/linux/device.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b093405..f08c25b 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -256,6 +256,9 @@ enum probe_type {
>   *		automatically.
>   * @pm:		Power management operations of the device which matched
>   *		this driver.
> + * @coredump:	Called when sysfs entry is written to. The device driver
> + *		is expected to call the dev_coredump API resulting in a
> + *		uevent.
>   * @p:		Driver core's private data, no one other than the driver
>   *		core can touch this.
>   *
> @@ -287,7 +290,7 @@ struct device_driver {
>  	const struct attribute_group **groups;
> 
>  	const struct dev_pm_ops *pm;
> -	int (*coredump) (struct device *dev);
> +	void (*coredump) (struct device *dev);

Isn't this going to cause build warnings now?  Are there no users of
this callback function yet?

thanks,

greg k-h

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Arend van Spriel]

On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
>> Upon submitting a patch for mwifiex [1] it was discussed whether this
>> callback function could fail. To keep things simple there is no need
>> for the error code so the driver can do the task synchronous or not
>> without worries. Currently the device driver core already ignores the
>> return value so changing it to void.
>>
>> [1] https://patchwork.kernel.org/patch/10231933/
>>
>> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
>> ---
>>  include/linux/device.h | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index b093405..f08c25b 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -256,6 +256,9 @@ enum probe_type {
>>   *           automatically.
>>   * @pm:              Power management operations of the device which matched
>>   *           this driver.
>> + * @coredump:        Called when sysfs entry is written to. The device driver
>> + *           is expected to call the dev_coredump API resulting in a
>> + *           uevent.
>>   * @p:               Driver core's private data, no one other than the driver
>>   *           core can touch this.
>>   *
>> @@ -287,7 +290,7 @@ struct device_driver {
>>       const struct attribute_group **groups;
>>
>>       const struct dev_pm_ops *pm;
>> -     int (*coredump) (struct device *dev);
>> +     void (*coredump) (struct device *dev);
>
> Isn't this going to cause build warnings now?  Are there no users of
> this callback function yet?

Hi Greg,

I submitted driver patches for the 4.17 kernel and from that
discussion we concluded it would be good to change to void return
type. So those driver patches were dropped. The caller of the callback
in drivers/base/dd.c does not use the return value so from that side
there is no issue. So my motivation for asking to consider this for
4.16 is so I can resubmit the driver patches for 4.17 if there is
still time before the merge window.

Regards,
Arend

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Greg Kroah-Hartman]

On Sat, Mar 24, 2018 at 09:50:05AM +0100, Arend van Spriel wrote:
> On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
> >> Upon submitting a patch for mwifiex [1] it was discussed whether this
> >> callback function could fail. To keep things simple there is no need
> >> for the error code so the driver can do the task synchronous or not
> >> without worries. Currently the device driver core already ignores the
> >> return value so changing it to void.
> >>
> >> [1] https://patchwork.kernel.org/patch/10231933/
> >>
> >> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
> >> ---
> >>  include/linux/device.h | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> index b093405..f08c25b 100644
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -256,6 +256,9 @@ enum probe_type {
> >>   *           automatically.
> >>   * @pm:              Power management operations of the device which matched
> >>   *           this driver.
> >> + * @coredump:        Called when sysfs entry is written to. The device driver
> >> + *           is expected to call the dev_coredump API resulting in a
> >> + *           uevent.
> >>   * @p:               Driver core's private data, no one other than the driver
> >>   *           core can touch this.
> >>   *
> >> @@ -287,7 +290,7 @@ struct device_driver {
> >>       const struct attribute_group **groups;
> >>
> >>       const struct dev_pm_ops *pm;
> >> -     int (*coredump) (struct device *dev);
> >> +     void (*coredump) (struct device *dev);
> >
> > Isn't this going to cause build warnings now?  Are there no users of
> > this callback function yet?
> 
> Hi Greg,
> 
> I submitted driver patches for the 4.17 kernel and from that
> discussion we concluded it would be good to change to void return
> type. So those driver patches were dropped. The caller of the callback
> in drivers/base/dd.c does not use the return value so from that side
> there is no issue. So my motivation for asking to consider this for
> 4.16 is so I can resubmit the driver patches for 4.17 if there is
> still time before the merge window.

It's too late for 4.16 for this, and I would queue it up in my tree now
but I don't want to cause any build warnings in linux-next from it.  So
how about I submit something like this right after 4.17-rc1 is out,
where the function signature is changed _and_ all definitions of that
function are changed at the same time to keep everything sane at once?

Can you send me such a patch right before -rc1 is out base on Linus's
tree?  That should give everyone enough time to get the things merged,
right?

Or is there no in-flight patches to use this yet, and I can queue it up
now for -rc1 as no build warnings will happen?

thanks,

greg k-h

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Arend van Spriel]

+ Marcel

On Sat, Mar 24, 2018 at 10:04 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Mar 24, 2018 at 09:50:05AM +0100, Arend van Spriel wrote:
>> On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
>> >> Upon submitting a patch for mwifiex [1] it was discussed whether this
>> >> callback function could fail. To keep things simple there is no need
>> >> for the error code so the driver can do the task synchronous or not
>> >> without worries. Currently the device driver core already ignores the
>> >> return value so changing it to void.
>> >>
>> >> [1] https://patchwork.kernel.org/patch/10231933/
>> >>
>> >> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
>> >> ---
>> >>  include/linux/device.h | 5 ++++-
>> >>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/include/linux/device.h b/include/linux/device.h
>> >> index b093405..f08c25b 100644
>> >> --- a/include/linux/device.h
>> >> +++ b/include/linux/device.h
>> >> @@ -256,6 +256,9 @@ enum probe_type {
>> >>   *           automatically.
>> >>   * @pm:              Power management operations of the device which matched
>> >>   *           this driver.
>> >> + * @coredump:        Called when sysfs entry is written to. The device driver
>> >> + *           is expected to call the dev_coredump API resulting in a
>> >> + *           uevent.
>> >>   * @p:               Driver core's private data, no one other than the driver
>> >>   *           core can touch this.
>> >>   *
>> >> @@ -287,7 +290,7 @@ struct device_driver {
>> >>       const struct attribute_group **groups;
>> >>
>> >>       const struct dev_pm_ops *pm;
>> >> -     int (*coredump) (struct device *dev);
>> >> +     void (*coredump) (struct device *dev);
>> >
>> > Isn't this going to cause build warnings now?  Are there no users of
>> > this callback function yet?
>>
>> Hi Greg,
>>
>> I submitted driver patches for the 4.17 kernel and from that
>> discussion we concluded it would be good to change to void return
>> type. So those driver patches were dropped. The caller of the callback
>> in drivers/base/dd.c does not use the return value so from that side
>> there is no issue. So my motivation for asking to consider this for
>> 4.16 is so I can resubmit the driver patches for 4.17 if there is
>> still time before the merge window.
>
> It's too late for 4.16 for this, and I would queue it up in my tree now
> but I don't want to cause any build warnings in linux-next from it.  So
> how about I submit something like this right after 4.17-rc1 is out,
> where the function signature is changed _and_ all definitions of that
> function are changed at the same time to keep everything sane at once?
>
> Can you send me such a patch right before -rc1 is out base on Linus's
> tree?  That should give everyone enough time to get the things merged,
> right?
>
> Or is there no in-flight patches to use this yet, and I can queue it up
> now for -rc1 as no build warnings will happen?

I had patches for wireless and bluetooth. The latter was already
applied by Marcel, but I asked to revert that. However, I just checked
bt-next to be sure and it is still there.

@Marcel: Can you revert commit 118c193d9699 ("btmrvl: support sysfs
initiated firmware coredump") [1]

With that out of the way there are no in-flight patches yet.

Regards,
Arend

[1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=118c193d

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Arend van Spriel]

On Sat, Mar 24, 2018 at 10:04 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Mar 24, 2018 at 09:50:05AM +0100, Arend van Spriel wrote:
>> On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
>> >> Upon submitting a patch for mwifiex [1] it was discussed whether this
>> >> callback function could fail. To keep things simple there is no need
>> >> for the error code so the driver can do the task synchronous or not
>> >> without worries. Currently the device driver core already ignores the
>> >> return value so changing it to void.
>> >>
>> >> [1] https://patchwork.kernel.org/patch/10231933/
>> >>
>> >> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
>> >> ---
>> >>  include/linux/device.h | 5 ++++-
>> >>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/include/linux/device.h b/include/linux/device.h
>> >> index b093405..f08c25b 100644
>> >> --- a/include/linux/device.h
>> >> +++ b/include/linux/device.h
>> >> @@ -256,6 +256,9 @@ enum probe_type {
>> >>   *           automatically.
>> >>   * @pm:              Power management operations of the device which matched
>> >>   *           this driver.
>> >> + * @coredump:        Called when sysfs entry is written to. The device driver
>> >> + *           is expected to call the dev_coredump API resulting in a
>> >> + *           uevent.
>> >>   * @p:               Driver core's private data, no one other than the driver
>> >>   *           core can touch this.
>> >>   *
>> >> @@ -287,7 +290,7 @@ struct device_driver {
>> >>       const struct attribute_group **groups;
>> >>
>> >>       const struct dev_pm_ops *pm;
>> >> -     int (*coredump) (struct device *dev);
>> >> +     void (*coredump) (struct device *dev);
>> >
>> > Isn't this going to cause build warnings now?  Are there no users of
>> > this callback function yet?
>>
>> Hi Greg,
>>
>> I submitted driver patches for the 4.17 kernel and from that
>> discussion we concluded it would be good to change to void return
>> type. So those driver patches were dropped. The caller of the callback
>> in drivers/base/dd.c does not use the return value so from that side
>> there is no issue. So my motivation for asking to consider this for
>> 4.16 is so I can resubmit the driver patches for 4.17 if there is
>> still time before the merge window.
>
> It's too late for 4.16 for this, and I would queue it up in my tree now
> but I don't want to cause any build warnings in linux-next from it.  So
> how about I submit something like this right after 4.17-rc1 is out,
> where the function signature is changed _and_ all definitions of that
> function are changed at the same time to keep everything sane at once?
>
> Can you send me such a patch right before -rc1 is out base on Linus's
> tree?  That should give everyone enough time to get the things merged,
> right?
>
> Or is there no in-flight patches to use this yet, and I can queue it up
> now for -rc1 as no build warnings will happen?

Hi Greg,

Are we good regarding this patch. I have assured there are not
in-flight patches.

Regards,
Arend

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Greg Kroah-Hartman]

On Fri, Apr 06, 2018 at 12:13:38PM +0200, Arend van Spriel wrote:
> On Sat, Mar 24, 2018 at 10:04 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Mar 24, 2018 at 09:50:05AM +0100, Arend van Spriel wrote:
> >> On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
> >> >> Upon submitting a patch for mwifiex [1] it was discussed whether this
> >> >> callback function could fail. To keep things simple there is no need
> >> >> for the error code so the driver can do the task synchronous or not
> >> >> without worries. Currently the device driver core already ignores the
> >> >> return value so changing it to void.
> >> >>
> >> >> [1] https://patchwork.kernel.org/patch/10231933/
> >> >>
> >> >> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
> >> >> ---
> >> >>  include/linux/device.h | 5 ++++-
> >> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> >> index b093405..f08c25b 100644
> >> >> --- a/include/linux/device.h
> >> >> +++ b/include/linux/device.h
> >> >> @@ -256,6 +256,9 @@ enum probe_type {
> >> >>   *           automatically.
> >> >>   * @pm:              Power management operations of the device which matched
> >> >>   *           this driver.
> >> >> + * @coredump:        Called when sysfs entry is written to. The device driver
> >> >> + *           is expected to call the dev_coredump API resulting in a
> >> >> + *           uevent.
> >> >>   * @p:               Driver core's private data, no one other than the driver
> >> >>   *           core can touch this.
> >> >>   *
> >> >> @@ -287,7 +290,7 @@ struct device_driver {
> >> >>       const struct attribute_group **groups;
> >> >>
> >> >>       const struct dev_pm_ops *pm;
> >> >> -     int (*coredump) (struct device *dev);
> >> >> +     void (*coredump) (struct device *dev);
> >> >
> >> > Isn't this going to cause build warnings now?  Are there no users of
> >> > this callback function yet?
> >>
> >> Hi Greg,
> >>
> >> I submitted driver patches for the 4.17 kernel and from that
> >> discussion we concluded it would be good to change to void return
> >> type. So those driver patches were dropped. The caller of the callback
> >> in drivers/base/dd.c does not use the return value so from that side
> >> there is no issue. So my motivation for asking to consider this for
> >> 4.16 is so I can resubmit the driver patches for 4.17 if there is
> >> still time before the merge window.
> >
> > It's too late for 4.16 for this, and I would queue it up in my tree now
> > but I don't want to cause any build warnings in linux-next from it.  So
> > how about I submit something like this right after 4.17-rc1 is out,
> > where the function signature is changed _and_ all definitions of that
> > function are changed at the same time to keep everything sane at once?
> >
> > Can you send me such a patch right before -rc1 is out base on Linus's
> > tree?  That should give everyone enough time to get the things merged,
> > right?
> >
> > Or is there no in-flight patches to use this yet, and I can queue it up
> > now for -rc1 as no build warnings will happen?
> 
> Hi Greg,
> 
> Are we good regarding this patch. I have assured there are not
> in-flight patches.

All of my stuff is now in Linus's tree, so check there :)

thanks,

greg k-h

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Arend van Spriel]

Op vr 6 apr. 2018 4:46 PM schreef Greg Kroah-Hartman
<gregkh@linuxfoundation.org>:
>
> On Fri, Apr 06, 2018 at 12:13:38PM +0200, Arend van Spriel wrote:
> > On Sat, Mar 24, 2018 at 10:04 AM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Mar 24, 2018 at 09:50:05AM +0100, Arend van Spriel wrote:
> > >> On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
> > >> <gregkh@linuxfoundation.org> wrote:
> > >> > On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
> > >> >> Upon submitting a patch for mwifiex [1] it was discussed whether this
> > >> >> callback function could fail. To keep things simple there is no need
> > >> >> for the error code so the driver can do the task synchronous or not
> > >> >> without worries. Currently the device driver core already ignores the
> > >> >> return value so changing it to void.
> > >> >>
> > >> >> [1] https://patchwork.kernel.org/patch/10231933/
> > >> >>
> > >> >> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
> > >> >> ---
> > >> >>  include/linux/device.h | 5 ++++-
> > >> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> > >> >>
> > >> >> diff --git a/include/linux/device.h b/include/linux/device.h
> > >> >> index b093405..f08c25b 100644
> > >> >> --- a/include/linux/device.h
> > >> >> +++ b/include/linux/device.h
> > >> >> @@ -256,6 +256,9 @@ enum probe_type {
> > >> >>   *           automatically.
> > >> >>   * @pm:              Power management operations of the device which matched
> > >> >>   *           this driver.
> > >> >> + * @coredump:        Called when sysfs entry is written to. The device driver
> > >> >> + *           is expected to call the dev_coredump API resulting in a
> > >> >> + *           uevent.
> > >> >>   * @p:               Driver core's private data, no one other than the driver
> > >> >>   *           core can touch this.
> > >> >>   *
> > >> >> @@ -287,7 +290,7 @@ struct device_driver {
> > >> >>       const struct attribute_group **groups;
> > >> >>
> > >> >>       const struct dev_pm_ops *pm;
> > >> >> -     int (*coredump) (struct device *dev);
> > >> >> +     void (*coredump) (struct device *dev);
> > >> >
> > >> > Isn't this going to cause build warnings now?  Are there no users of
> > >> > this callback function yet?
> > >>
> > >> Hi Greg,
> > >>
> > >> I submitted driver patches for the 4.17 kernel and from that
> > >> discussion we concluded it would be good to change to void return
> > >> type. So those driver patches were dropped. The caller of the callback
> > >> in drivers/base/dd.c does not use the return value so from that side
> > >> there is no issue. So my motivation for asking to consider this for
> > >> 4.16 is so I can resubmit the driver patches for 4.17 if there is
> > >> still time before the merge window.
> > >
> > > It's too late for 4.16 for this, and I would queue it up in my tree now
> > > but I don't want to cause any build warnings in linux-next from it.  So
> > > how about I submit something like this right after 4.17-rc1 is out,
> > > where the function signature is changed _and_ all definitions of that
> > > function are changed at the same time to keep everything sane at once?
> > >
> > > Can you send me such a patch right before -rc1 is out base on Linus's
> > > tree?  That should give everyone enough time to get the things merged,
> > > right?
> > >
> > > Or is there no in-flight patches to use this yet, and I can queue it up
> > > now for -rc1 as no build warnings will happen?
> >
> > Hi Greg,
> >
> > Are we good regarding this patch. I have assured there are not
> > in-flight patches.
>
> All of my stuff is now in Linus's tree, so check there :)

Hi Greg,

That should have been obvious to me with merge window. Anyway, I
checked and it seems this particular patch got lost somehow. The other
two patches in the series are in Linus's tree. There was only one
in-flight patch in bt-next and had it removed with you on Cc:.
Probably got lost in your daily email storm :-p

Regards,
Arend

Re: [PATCH for-4.16 2/3] drivers: change struct device_driver::coredump() return type to void

[Greg Kroah-Hartman]

On Fri, Apr 06, 2018 at 09:35:35PM +0200, Arend van Spriel wrote:
> Op vr 6 apr. 2018 4:46 PM schreef Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>:
> >
> > On Fri, Apr 06, 2018 at 12:13:38PM +0200, Arend van Spriel wrote:
> > > On Sat, Mar 24, 2018 at 10:04 AM, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Sat, Mar 24, 2018 at 09:50:05AM +0100, Arend van Spriel wrote:
> > > >> On Fri, Mar 23, 2018 at 5:55 PM, Greg Kroah-Hartman
> > > >> <gregkh@linuxfoundation.org> wrote:
> > > >> > On Thu, Mar 15, 2018 at 10:55:24AM +0100, Arend van Spriel wrote:
> > > >> >> Upon submitting a patch for mwifiex [1] it was discussed whether this
> > > >> >> callback function could fail. To keep things simple there is no need
> > > >> >> for the error code so the driver can do the task synchronous or not
> > > >> >> without worries. Currently the device driver core already ignores the
> > > >> >> return value so changing it to void.
> > > >> >>
> > > >> >> [1] https://patchwork.kernel.org/patch/10231933/
> > > >> >>
> > > >> >> Signed-off-by: Arend van Spriel <aspriel@gmail.com>
> > > >> >> ---
> > > >> >>  include/linux/device.h | 5 ++++-
> > > >> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >> >>
> > > >> >> diff --git a/include/linux/device.h b/include/linux/device.h
> > > >> >> index b093405..f08c25b 100644
> > > >> >> --- a/include/linux/device.h
> > > >> >> +++ b/include/linux/device.h
> > > >> >> @@ -256,6 +256,9 @@ enum probe_type {
> > > >> >>   *           automatically.
> > > >> >>   * @pm:              Power management operations of the device which matched
> > > >> >>   *           this driver.
> > > >> >> + * @coredump:        Called when sysfs entry is written to. The device driver
> > > >> >> + *           is expected to call the dev_coredump API resulting in a
> > > >> >> + *           uevent.
> > > >> >>   * @p:               Driver core's private data, no one other than the driver
> > > >> >>   *           core can touch this.
> > > >> >>   *
> > > >> >> @@ -287,7 +290,7 @@ struct device_driver {
> > > >> >>       const struct attribute_group **groups;
> > > >> >>
> > > >> >>       const struct dev_pm_ops *pm;
> > > >> >> -     int (*coredump) (struct device *dev);
> > > >> >> +     void (*coredump) (struct device *dev);
> > > >> >
> > > >> > Isn't this going to cause build warnings now?  Are there no users of
> > > >> > this callback function yet?
> > > >>
> > > >> Hi Greg,
> > > >>
> > > >> I submitted driver patches for the 4.17 kernel and from that
> > > >> discussion we concluded it would be good to change to void return
> > > >> type. So those driver patches were dropped. The caller of the callback
> > > >> in drivers/base/dd.c does not use the return value so from that side
> > > >> there is no issue. So my motivation for asking to consider this for
> > > >> 4.16 is so I can resubmit the driver patches for 4.17 if there is
> > > >> still time before the merge window.
> > > >
> > > > It's too late for 4.16 for this, and I would queue it up in my tree now
> > > > but I don't want to cause any build warnings in linux-next from it.  So
> > > > how about I submit something like this right after 4.17-rc1 is out,
> > > > where the function signature is changed _and_ all definitions of that
> > > > function are changed at the same time to keep everything sane at once?
> > > >
> > > > Can you send me such a patch right before -rc1 is out base on Linus's
> > > > tree?  That should give everyone enough time to get the things merged,
> > > > right?
> > > >
> > > > Or is there no in-flight patches to use this yet, and I can queue it up
> > > > now for -rc1 as no build warnings will happen?
> > >
> > > Hi Greg,
> > >
> > > Are we good regarding this patch. I have assured there are not
> > > in-flight patches.
> >
> > All of my stuff is now in Linus's tree, so check there :)
> 
> Hi Greg,
> 
> That should have been obvious to me with merge window. Anyway, I
> checked and it seems this particular patch got lost somehow. The other
> two patches in the series are in Linus's tree. There was only one
> in-flight patch in bt-next and had it removed with you on Cc:.
> Probably got lost in your daily email storm :-p

Ah, sorry, now I remember.  Yeah, this is long-gone from my tree, care
to resend this and I'll get it merged after 4.17-rc1 is out?

thanks,

greg k-h

[PATCH for-4.16 3/3] drivers: base: remove check for callback in coredump_store()

[Arend van Spriel]

The check for the .coredump() callback in coredump_store() is
redundant. It is already assured the device driver implements
the callback upon creating the coredump sysfs entry.

Signed-off-by: Arend van Spriel <aspriel@gmail.com>
---
 drivers/base/dd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index de6fd09..c9f5408 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -292,8 +292,7 @@ static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
 	device_lock(dev);
-	if (dev->driver->coredump)
-		dev->driver->coredump(dev);
+	dev->driver->coredump(dev);
 	device_unlock(dev);

 	return count;
--
1.9.1

Re: [PATCH for-4.16 0/3] drivers: base: revise coredump ABI

[Greg Kroah-Hartman]

On Thu, Mar 15, 2018 at 10:55:22AM +0100, Arend van Spriel wrote:
> Not sure if this can be considered as fix for v4.16. Upon submitting
> the driver patches I received valuable feedback that I would like to
> address with this series.
> 
> The patch apply to master and driver-core-next of the driver-core
> repository so you can take it either way.

We can wait for 4.17-rc1 for these, they are not fixing regressions.

thanks,

greg k-h