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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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