| Message ID | 20200925161929.1136806-2-tasleson@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add persistent durable identifier to storage log messages | expand |
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
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
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---
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 --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) {
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(+)