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@xxxxxxxxxx>
> > ---
> > 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
