diff mbox series

[v5,01/12] struct device: Add function callback durable_name

Message ID 20200925161929.1136806-2-tasleson@redhat.com
State New
Headers show
Series Add persistent durable identifier to storage log messages | expand

Commit Message

Tony Asleson Sept. 25, 2020, 4:19 p.m. UTC
Function callback and function to be used to write a persistent
durable name to the supplied character buffer.  This will be used to add
structured key-value data to log messages for hardware related errors
which allows end users to correlate message and specific hardware.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 drivers/base/core.c    | 24 ++++++++++++++++++++++++
 include/linux/device.h |  4 ++++
 2 files changed, 28 insertions(+)

Comments

Sergei Shtylyov Sept. 26, 2020, 9:08 a.m. UTC | #1
On 25.09.2020 19:19, Tony Asleson wrote:

> Function callback and function to be used to write a persistent

> durable name to the supplied character buffer.  This will be used to add

> structured key-value data to log messages for hardware related errors

> which allows end users to correlate message and specific hardware.

> 

> Signed-off-by: Tony Asleson <tasleson@redhat.com>

> ---

>   drivers/base/core.c    | 24 ++++++++++++++++++++++++

>   include/linux/device.h |  4 ++++

>   2 files changed, 28 insertions(+)

> 

> diff --git a/drivers/base/core.c b/drivers/base/core.c

> index 05d414e9e8a4..88696ade8bfc 100644

> --- a/drivers/base/core.c

> +++ b/drivers/base/core.c

> @@ -2489,6 +2489,30 @@ int dev_set_name(struct device *dev, const char *fmt, ...)

>   }

>   EXPORT_SYMBOL_GPL(dev_set_name);

>   

> +/**

> + * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer

> + * @dev: device

> + * @buffer: character buffer to write results

> + * @len: length of buffer

> + * @return: Number of bytes written to buffer


    This is not how the kernel-doc commenta describe the function result, IIRC...

> + */

> +int dev_durable_name(const struct device *dev, char *buffer, size_t len)

> +{

> +	int tmp, dlen;

> +

> +	if (dev && dev->durable_name) {

> +		tmp = snprintf(buffer, len, "DURABLE_NAME=");

> +		if (tmp < len) {

> +			dlen = dev->durable_name(dev, buffer + tmp,

> +							len - tmp);

> +			if (dlen > 0 && ((dlen + tmp) < len))

> +				return dlen + tmp;

> +		}

> +	}

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(dev_durable_name);

> +

>   /**

>    * device_to_dev_kobj - select a /sys/dev/ directory for the device

>    * @dev: device

[...]

MBR, Sergei
Tony Asleson Sept. 27, 2020, 2:22 p.m. UTC | #2
On 9/26/20 4:08 AM, Sergei Shtylyov wrote:
> On 25.09.2020 19:19, Tony Asleson wrote:

> 

>> Function callback and function to be used to write a persistent

>> durable name to the supplied character buffer.  This will be used to add

>> structured key-value data to log messages for hardware related errors

>> which allows end users to correlate message and specific hardware.

>>

>> Signed-off-by: Tony Asleson <tasleson@redhat.com>

>> ---

>>   drivers/base/core.c    | 24 ++++++++++++++++++++++++

>>   include/linux/device.h |  4 ++++

>>   2 files changed, 28 insertions(+)

>>

>> diff --git a/drivers/base/core.c b/drivers/base/core.c

>> index 05d414e9e8a4..88696ade8bfc 100644

>> --- a/drivers/base/core.c

>> +++ b/drivers/base/core.c

>> @@ -2489,6 +2489,30 @@ int dev_set_name(struct device *dev, const char

>> *fmt, ...)

>>   }

>>   EXPORT_SYMBOL_GPL(dev_set_name);

>>   +/**

>> + * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer

>> + * @dev: device

>> + * @buffer: character buffer to write results

>> + * @len: length of buffer

>> + * @return: Number of bytes written to buffer

> 

>    This is not how the kernel-doc commenta describe the function result,

> IIRC...


I did my compile with `make  W=1` and there isn't any warnings/error
with source documentation, but the documentation does indeed outline a
different syntax.  It's interesting how common the @return syntax is in
the existing code base.

I'll re-work the function documentation return.

Thanks
Christoph Hellwig Sept. 29, 2020, 5:51 p.m. UTC | #3
Independ of my opinion on the whole scheme that I shared last time,
we really should not bloat struct device with function pointers.

On Fri, Sep 25, 2020 at 11:19:18AM -0500, Tony Asleson wrote:
> Function callback and function to be used to write a persistent

> durable name to the supplied character buffer.  This will be used to add

> structured key-value data to log messages for hardware related errors

> which allows end users to correlate message and specific hardware.

> 

> Signed-off-by: Tony Asleson <tasleson@redhat.com>

> ---

>  drivers/base/core.c    | 24 ++++++++++++++++++++++++

>  include/linux/device.h |  4 ++++

>  2 files changed, 28 insertions(+)

> 

> diff --git a/drivers/base/core.c b/drivers/base/core.c

> index 05d414e9e8a4..88696ade8bfc 100644

> --- a/drivers/base/core.c

> +++ b/drivers/base/core.c

> @@ -2489,6 +2489,30 @@ int dev_set_name(struct device *dev, const char *fmt, ...)

>  }

>  EXPORT_SYMBOL_GPL(dev_set_name);

>  

> +/**

> + * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer

> + * @dev: device

> + * @buffer: character buffer to write results

> + * @len: length of buffer

> + * @return: Number of bytes written to buffer

> + */

> +int dev_durable_name(const struct device *dev, char *buffer, size_t len)

> +{

> +	int tmp, dlen;

> +

> +	if (dev && dev->durable_name) {

> +		tmp = snprintf(buffer, len, "DURABLE_NAME=");

> +		if (tmp < len) {

> +			dlen = dev->durable_name(dev, buffer + tmp,

> +							len - tmp);

> +			if (dlen > 0 && ((dlen + tmp) < len))

> +				return dlen + tmp;

> +		}

> +	}

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(dev_durable_name);

> +

>  /**

>   * device_to_dev_kobj - select a /sys/dev/ directory for the device

>   * @dev: device

> diff --git a/include/linux/device.h b/include/linux/device.h

> index 5efed864b387..074125999dd8 100644

> --- a/include/linux/device.h

> +++ b/include/linux/device.h

> @@ -614,6 +614,8 @@ struct device {

>  	struct iommu_group	*iommu_group;

>  	struct dev_iommu	*iommu;

>  

> +	int (*durable_name)(const struct device *dev, char *buff, size_t len);

> +

>  	bool			offline_disabled:1;

>  	bool			offline:1;

>  	bool			of_node_reused:1;

> @@ -655,6 +657,8 @@ static inline const char *dev_name(const struct device *dev)

>  extern __printf(2, 3)

>  int dev_set_name(struct device *dev, const char *name, ...);

>  

> +int dev_durable_name(const struct device *d, char *buffer, size_t len);

> +

>  #ifdef CONFIG_NUMA

>  static inline int dev_to_node(struct device *dev)

>  {

> -- 

> 2.26.2

> 

---end quoted text---
Greg Kroah-Hartman Sept. 29, 2020, 6:04 p.m. UTC | #4
On Tue, Sep 29, 2020 at 06:51:02PM +0100, Christoph Hellwig wrote:
> Independ of my opinion on the whole scheme that I shared last time,

> we really should not bloat struct device with function pointers.

> 

> On Fri, Sep 25, 2020 at 11:19:18AM -0500, Tony Asleson wrote:

> > Function callback and function to be used to write a persistent

> > durable name to the supplied character buffer.  This will be used to add

> > structured key-value data to log messages for hardware related errors

> > which allows end users to correlate message and specific hardware.

> > 

> > Signed-off-by: Tony Asleson <tasleson@redhat.com>

> > ---

> >  drivers/base/core.c    | 24 ++++++++++++++++++++++++

> >  include/linux/device.h |  4 ++++

> >  2 files changed, 28 insertions(+)


I can't find this patch anywhere in my archives, why was I not cc:ed on
it?  It's a v5 and no one thought to ask the driver core
developers/maintainers about it???

{sigh}

And for log messages, what about the dynamic debug developers, why not
include them as well?  Since when is this a storage-only thing?

> > 

> > diff --git a/drivers/base/core.c b/drivers/base/core.c

> > index 05d414e9e8a4..88696ade8bfc 100644

> > --- a/drivers/base/core.c

> > +++ b/drivers/base/core.c

> > @@ -2489,6 +2489,30 @@ int dev_set_name(struct device *dev, const char *fmt, ...)

> >  }

> >  EXPORT_SYMBOL_GPL(dev_set_name);

> >  

> > +/**

> > + * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer

> > + * @dev: device

> > + * @buffer: character buffer to write results

> > + * @len: length of buffer

> > + * @return: Number of bytes written to buffer

> > + */

> > +int dev_durable_name(const struct device *dev, char *buffer, size_t len)

> > +{

> > +	int tmp, dlen;

> > +

> > +	if (dev && dev->durable_name) {

> > +		tmp = snprintf(buffer, len, "DURABLE_NAME=");

> > +		if (tmp < len) {

> > +			dlen = dev->durable_name(dev, buffer + tmp,

> > +							len - tmp);

> > +			if (dlen > 0 && ((dlen + tmp) < len))

> > +				return dlen + tmp;

> > +		}

> > +	}

> > +	return 0;

> > +}

> > +EXPORT_SYMBOL_GPL(dev_durable_name);

> > +

> >  /**

> >   * device_to_dev_kobj - select a /sys/dev/ directory for the device

> >   * @dev: device

> > diff --git a/include/linux/device.h b/include/linux/device.h

> > index 5efed864b387..074125999dd8 100644

> > --- a/include/linux/device.h

> > +++ b/include/linux/device.h

> > @@ -614,6 +614,8 @@ struct device {

> >  	struct iommu_group	*iommu_group;

> >  	struct dev_iommu	*iommu;

> >  

> > +	int (*durable_name)(const struct device *dev, char *buff, size_t len);


No, that's not ok at all, why is this even a thing?

Who is setting this?  Why can't the bus do this without anything
"special" needed from the driver core?

We have a mapping of 'struct device' to a unique hardware device at a
specific point in time, why are you trying to create another one?

What is wrong with what we have today?

So this is a HARD NAK on this patch for now.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 05d414e9e8a4..88696ade8bfc 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2489,6 +2489,30 @@  int dev_set_name(struct device *dev, const char *fmt, ...)
 }
 EXPORT_SYMBOL_GPL(dev_set_name);
 
+/**
+ * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer
+ * @dev: device
+ * @buffer: character buffer to write results
+ * @len: length of buffer
+ * @return: Number of bytes written to buffer
+ */
+int dev_durable_name(const struct device *dev, char *buffer, size_t len)
+{
+	int tmp, dlen;
+
+	if (dev && dev->durable_name) {
+		tmp = snprintf(buffer, len, "DURABLE_NAME=");
+		if (tmp < len) {
+			dlen = dev->durable_name(dev, buffer + tmp,
+							len - tmp);
+			if (dlen > 0 && ((dlen + tmp) < len))
+				return dlen + tmp;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_durable_name);
+
 /**
  * device_to_dev_kobj - select a /sys/dev/ directory for the device
  * @dev: device
diff --git a/include/linux/device.h b/include/linux/device.h
index 5efed864b387..074125999dd8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -614,6 +614,8 @@  struct device {
 	struct iommu_group	*iommu_group;
 	struct dev_iommu	*iommu;
 
+	int (*durable_name)(const struct device *dev, char *buff, size_t len);
+
 	bool			offline_disabled:1;
 	bool			offline:1;
 	bool			of_node_reused:1;
@@ -655,6 +657,8 @@  static inline const char *dev_name(const struct device *dev)
 extern __printf(2, 3)
 int dev_set_name(struct device *dev, const char *name, ...);
 
+int dev_durable_name(const struct device *d, char *buffer, size_t len);
+
 #ifdef CONFIG_NUMA
 static inline int dev_to_node(struct device *dev)
 {