diff mbox series

[v13,12/18] platform: Add __free() based cleanup function for platform_device_put

Message ID 20241009124120.1124-13-shiju.jose@huawei.com
State New
Headers show
Series EDAC: Scrub: introduce generic EDAC RAS control feature driver + CXL/ACPI-RAS2 drivers | expand

Commit Message

Shiju Jose Oct. 9, 2024, 12:41 p.m. UTC
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Add __free() based cleanup function for platform_device_put().

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 include/linux/platform_device.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Jonathan Cameron Oct. 14, 2024, 3:43 p.m. UTC | #1
On Wed, 9 Oct 2024 13:41:13 +0100
<shiju.jose@huawei.com> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Add __free() based cleanup function for platform_device_put().
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  include/linux/platform_device.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index d422db6eec63..606533b88f44 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
>  extern int platform_device_add(struct platform_device *pdev);
>  extern void platform_device_del(struct platform_device *pdev);
>  extern void platform_device_put(struct platform_device *pdev);
> +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
>  
>  struct platform_driver {
>  	int (*probe)(struct platform_device *);

+CC Greg KH and Rafael.

Makes sure to include them on v14 as this needs review from a driver core point
of view I think.
Greg KH Oct. 14, 2024, 4 p.m. UTC | #2
On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:
> On Wed, 9 Oct 2024 13:41:13 +0100
> <shiju.jose@huawei.com> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Add __free() based cleanup function for platform_device_put().
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > ---
> >  include/linux/platform_device.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > index d422db6eec63..606533b88f44 100644
> > --- a/include/linux/platform_device.h
> > +++ b/include/linux/platform_device.h
> > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> >  extern int platform_device_add(struct platform_device *pdev);
> >  extern void platform_device_del(struct platform_device *pdev);
> >  extern void platform_device_put(struct platform_device *pdev);
> > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> >  
> >  struct platform_driver {
> >  	int (*probe)(struct platform_device *);
> 
> +CC Greg KH and Rafael.
> 
> Makes sure to include them on v14 as this needs review from a driver core point
> of view I think.

Why is this needed for a platform device?  This feels like you will have
to do more work to "keep" the reference on the normal path than you to
today to release the reference on the error path, right?  Have a pointer
to a patch that uses this?

thanks,

greg k-h
Greg KH Oct. 14, 2024, 4:04 p.m. UTC | #3
On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:
> On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:
> > On Wed, 9 Oct 2024 13:41:13 +0100
> > <shiju.jose@huawei.com> wrote:
> > 
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > Add __free() based cleanup function for platform_device_put().
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > ---
> > >  include/linux/platform_device.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > index d422db6eec63..606533b88f44 100644
> > > --- a/include/linux/platform_device.h
> > > +++ b/include/linux/platform_device.h
> > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > >  extern int platform_device_add(struct platform_device *pdev);
> > >  extern void platform_device_del(struct platform_device *pdev);
> > >  extern void platform_device_put(struct platform_device *pdev);
> > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > >  
> > >  struct platform_driver {
> > >  	int (*probe)(struct platform_device *);
> > 
> > +CC Greg KH and Rafael.
> > 
> > Makes sure to include them on v14 as this needs review from a driver core point
> > of view I think.
> 
> Why is this needed for a platform device?  This feels like you will have
> to do more work to "keep" the reference on the normal path than you to
> today to release the reference on the error path, right?  Have a pointer
> to a patch that uses this?

Ah, is it this one:
	https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
?

If so, no, that's an abuse of a platform device, don't do that, make a
REAL device on the bus that this device lives on.  If it doesn't live on
a real bus, then put it on the virtual bus but do NOT abuse the platform
device layer for something like this.

thanks,

greg k-h
Jonathan Cameron Oct. 14, 2024, 5:16 p.m. UTC | #4
On Mon, 14 Oct 2024 18:04:37 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:
> > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:  
> > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > <shiju.jose@huawei.com> wrote:
> > >   
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > Add __free() based cleanup function for platform_device_put().
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > ---
> > > >  include/linux/platform_device.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > index d422db6eec63..606533b88f44 100644
> > > > --- a/include/linux/platform_device.h
> > > > +++ b/include/linux/platform_device.h
> > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > >  extern int platform_device_add(struct platform_device *pdev);
> > > >  extern void platform_device_del(struct platform_device *pdev);
> > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > >  
> > > >  struct platform_driver {
> > > >  	int (*probe)(struct platform_device *);  
> > > 
> > > +CC Greg KH and Rafael.
> > > 
> > > Makes sure to include them on v14 as this needs review from a driver core point
> > > of view I think.  
> > 
> > Why is this needed for a platform device?  This feels like you will have
> > to do more work to "keep" the reference on the normal path than you to
> > today to release the reference on the error path, right?  Have a pointer
> > to a patch that uses this?  
> 
> Ah, is it this one:
> 	https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> ?
> 
> If so, no, that's an abuse of a platform device, don't do that, make a
> REAL device on the bus that this device lives on.  If it doesn't live on
> a real bus, then put it on the virtual bus but do NOT abuse the platform
> device layer for something like this.

Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
for a 'feature' that is described only by an ACPI table (here RAS2)?
Kind of similar(ish) to say IORT.

My thinking on a platform device was that this could be described
in DSDT and would have ended up as one. No idea why it isn't.
Maybe it predated the resource stuff that lets you use PCC channels
from methods under devices. Anyhow, it's not something I care about
so virtual bus is fine by me.

Jonathan


> 
> thanks,
> 
> greg k-h
>
Rafael J. Wysocki Oct. 14, 2024, 6:06 p.m. UTC | #5
On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 14 Oct 2024 18:04:37 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:
> > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:
> > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > <shiju.jose@huawei.com> wrote:
> > > >
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > >
> > > > > Add __free() based cleanup function for platform_device_put().
> > > > >
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > ---
> > > > >  include/linux/platform_device.h | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > index d422db6eec63..606533b88f44 100644
> > > > > --- a/include/linux/platform_device.h
> > > > > +++ b/include/linux/platform_device.h
> > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > >
> > > > >  struct platform_driver {
> > > > >         int (*probe)(struct platform_device *);
> > > >
> > > > +CC Greg KH and Rafael.
> > > >
> > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > of view I think.
> > >
> > > Why is this needed for a platform device?  This feels like you will have
> > > to do more work to "keep" the reference on the normal path than you to
> > > today to release the reference on the error path, right?  Have a pointer
> > > to a patch that uses this?
> >
> > Ah, is it this one:
> >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > ?
> >
> > If so, no, that's an abuse of a platform device, don't do that, make a
> > REAL device on the bus that this device lives on.  If it doesn't live on
> > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > device layer for something like this.
>
> Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> for a 'feature' that is described only by an ACPI table (here RAS2)?
> Kind of similar(ish) to say IORT.

Good question.

I guess it depends on whether or not there are any registers to access
or AML to interact with.  If so, I think that a platform device makes
sense.

> My thinking on a platform device was that this could be described
> in DSDT and would have ended up as one. No idea why it isn't.
> Maybe it predated the resource stuff that lets you use PCC channels
> from methods under devices. Anyhow, it's not something I care about
> so virtual bus is fine by me.
Jonathan Cameron Oct. 15, 2024, 9:10 a.m. UTC | #6
On Mon, 14 Oct 2024 20:06:40 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Mon, 14 Oct 2024 18:04:37 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >  
> > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:  
> > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:  
> > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > <shiju.jose@huawei.com> wrote:
> > > > >  
> > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > >
> > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > >
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > ---
> > > > > >  include/linux/platform_device.h | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > --- a/include/linux/platform_device.h
> > > > > > +++ b/include/linux/platform_device.h
> > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > >
> > > > > >  struct platform_driver {
> > > > > >         int (*probe)(struct platform_device *);  
> > > > >
> > > > > +CC Greg KH and Rafael.
> > > > >
> > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > of view I think.  
> > > >
> > > > Why is this needed for a platform device?  This feels like you will have
> > > > to do more work to "keep" the reference on the normal path than you to
> > > > today to release the reference on the error path, right?  Have a pointer
> > > > to a patch that uses this?  
> > >
> > > Ah, is it this one:
> > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > ?
> > >
> > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > device layer for something like this.  
> >
> > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > Kind of similar(ish) to say IORT.  
> 
> Good question.
> 
> I guess it depends on whether or not there are any registers to access
> or AML to interact with.  If so, I think that a platform device makes
> sense.

Unfortunately still a gray area I think.

This does access mailbox memory addresses, but they are provided
by an existing platform device, so maybe platform device for this
device is still inappropriate :(

What this uses is:
PCC channel (mailbox in memory + doorbells, etc but that is indirectly
provided as a service via reference in ACPI to the PCCT table entry
allowing this to find the mailbox device - which is a platform
device drivers/mailbox/pcc.c).
Because it's all spec defined content in the mailbox messages, we don't
have the more flexible (and newer I think) 'register' via operation region
stuff in AML.

A wrinkle though.  The mailbox data is mapped into this driver via
an acpi_os_ioremap() call.  

So I'm thinking we don't have a strong reason for a platform device
other than 'similarity' to other examples.  Never the strongest reason!

We'll explore alternatives and see what they end up looking like.

Jonathan



> 
> > My thinking on a platform device was that this could be described
> > in DSDT and would have ended up as one. No idea why it isn't.
> > Maybe it predated the resource stuff that lets you use PCC channels
> > from methods under devices. Anyhow, it's not something I care about
> > so virtual bus is fine by me.  
>
Jonathan Cameron Oct. 15, 2024, 9:40 a.m. UTC | #7
On Tue, 15 Oct 2024 10:10:25 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 14 Oct 2024 20:06:40 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> 
> > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > >  
> > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:  
> > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:  
> > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > <shiju.jose@huawei.com> wrote:
> > > > > >  
> > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > >
> > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > >
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > ---
> > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > --- a/include/linux/platform_device.h
> > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > >
> > > > > > >  struct platform_driver {
> > > > > > >         int (*probe)(struct platform_device *);  
> > > > > >
> > > > > > +CC Greg KH and Rafael.
> > > > > >
> > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > of view I think.  
> > > > >
> > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > to a patch that uses this?  
> > > >
> > > > Ah, is it this one:
> > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > ?
> > > >
> > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > device layer for something like this.  
> > >
> > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > Kind of similar(ish) to say IORT.  
> > 
> > Good question.
> > 
> > I guess it depends on whether or not there are any registers to access
> > or AML to interact with.  If so, I think that a platform device makes
> > sense.
> 
> Unfortunately still a gray area I think.
> 
> This does access mailbox memory addresses, but they are provided
> by an existing platform device, so maybe platform device for this
> device is still inappropriate :(
> 
> What this uses is:
> PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> provided as a service via reference in ACPI to the PCCT table entry
> allowing this to find the mailbox device - which is a platform
> device drivers/mailbox/pcc.c).
> Because it's all spec defined content in the mailbox messages, we don't
> have the more flexible (and newer I think) 'register' via operation region
> stuff in AML.
> 
> A wrinkle though.  The mailbox data is mapped into this driver via
> an acpi_os_ioremap() call.  
> 
> So I'm thinking we don't have a strong reason for a platform device
> other than 'similarity' to other examples.  Never the strongest reason!
> 
> We'll explore alternatives and see what they end up looking like.
> 
> Jonathan
> 

Greg,

I'm struggling a little to figure out how you envision the virtual bus
working here.  So before we spend too much time implementing the wrong thing
as it feels non trivial, let me check my understanding.

Would this mean registering a ras2 bus via subsys_virtual_register().
(Similar to done for memory tiers)

On that we'd then add all the devices: one per RAS2 PCC descriptor (these
are one per independent feature). Each feature has its own mailbox sub
channel (via a reference to the PCC mailbox devices .
Typically you have one of these per feature type per numa node, but
that isn't guaranteed.  That will then need wiring up with bus->probe() etc
so that the RAS2 edac feature drivers can find this later and bind to it to
register with edac etc.

So spinning up a full new bus, to support this?  I'm not against that.
We could use auxbus I guess but that is at least described
with the intent that it is for subfeatures of a device and here we don't have
a top level device (unless we make one for the RAS2 ACPI table which would
be odd).

So effectively a new subsystem bus?

Jonathan



> 
> 
> > 
> > > My thinking on a platform device was that this could be described
> > > in DSDT and would have ended up as one. No idea why it isn't.
> > > Maybe it predated the resource stuff that lets you use PCC channels
> > > from methods under devices. Anyhow, it's not something I care about
> > > so virtual bus is fine by me.  
> > 
> 
>
Greg KH Oct. 15, 2024, 10:17 a.m. UTC | #8
On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:
> On Tue, 15 Oct 2024 10:10:25 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Mon, 14 Oct 2024 20:06:40 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > 
> > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:
> > > >
> > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >  
> > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:  
> > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:  
> > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > >  
> > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > >
> > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > >
> > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > ---
> > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > >
> > > > > > > >  struct platform_driver {
> > > > > > > >         int (*probe)(struct platform_device *);  
> > > > > > >
> > > > > > > +CC Greg KH and Rafael.
> > > > > > >
> > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > of view I think.  
> > > > > >
> > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > to a patch that uses this?  
> > > > >
> > > > > Ah, is it this one:
> > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > ?
> > > > >
> > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > device layer for something like this.  
> > > >
> > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > Kind of similar(ish) to say IORT.  
> > > 
> > > Good question.
> > > 
> > > I guess it depends on whether or not there are any registers to access
> > > or AML to interact with.  If so, I think that a platform device makes
> > > sense.
> > 
> > Unfortunately still a gray area I think.
> > 
> > This does access mailbox memory addresses, but they are provided
> > by an existing platform device, so maybe platform device for this
> > device is still inappropriate :(
> > 
> > What this uses is:
> > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > provided as a service via reference in ACPI to the PCCT table entry
> > allowing this to find the mailbox device - which is a platform
> > device drivers/mailbox/pcc.c).
> > Because it's all spec defined content in the mailbox messages, we don't
> > have the more flexible (and newer I think) 'register' via operation region
> > stuff in AML.
> > 
> > A wrinkle though.  The mailbox data is mapped into this driver via
> > an acpi_os_ioremap() call.  
> > 
> > So I'm thinking we don't have a strong reason for a platform device
> > other than 'similarity' to other examples.  Never the strongest reason!
> > 
> > We'll explore alternatives and see what they end up looking like.
> > 
> > Jonathan
> > 
> 
> Greg,
> 
> I'm struggling a little to figure out how you envision the virtual bus
> working here.  So before we spend too much time implementing the wrong thing
> as it feels non trivial, let me check my understanding.
> 
> Would this mean registering a ras2 bus via subsys_virtual_register().
> (Similar to done for memory tiers)

It should show up under /sys/devices/virtual/ is what I mean.

> On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> are one per independent feature). Each feature has its own mailbox sub
> channel (via a reference to the PCC mailbox devices .
> Typically you have one of these per feature type per numa node, but
> that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> so that the RAS2 edac feature drivers can find this later and bind to it to
> register with edac etc.
> 
> So spinning up a full new bus, to support this?  I'm not against that.

No, again, see how the stuff that shows up in /sys/devices/virtual
works, that should be much simpler.

But really, as this is a "bus", just make a new one.  I don't understand
why ACPI isn't creating your devices for you, as this is ACPI code,
perhaps just fix that up instead?  That would make much more sense to
me...

thanks,

greg k-h
Rafael J. Wysocki Oct. 15, 2024, 1:32 p.m. UTC | #9
On Tue, Oct 15, 2024 at 12:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:
> > On Tue, 15 Oct 2024 10:10:25 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >
> > > On Mon, 14 Oct 2024 20:06:40 +0200
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >
> > > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > > <Jonathan.Cameron@huawei.com> wrote:
> > > > >
> > > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:
> > > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:
> > > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > > >
> > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > >
> > > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > > ---
> > > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > > >
> > > > > > > > >  struct platform_driver {
> > > > > > > > >         int (*probe)(struct platform_device *);
> > > > > > > >
> > > > > > > > +CC Greg KH and Rafael.
> > > > > > > >
> > > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > > of view I think.
> > > > > > >
> > > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > > to a patch that uses this?
> > > > > >
> > > > > > Ah, is it this one:
> > > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > > ?
> > > > > >
> > > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > > device layer for something like this.
> > > > >
> > > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > > Kind of similar(ish) to say IORT.
> > > >
> > > > Good question.
> > > >
> > > > I guess it depends on whether or not there are any registers to access
> > > > or AML to interact with.  If so, I think that a platform device makes
> > > > sense.
> > >
> > > Unfortunately still a gray area I think.
> > >
> > > This does access mailbox memory addresses, but they are provided
> > > by an existing platform device, so maybe platform device for this
> > > device is still inappropriate :(
> > >
> > > What this uses is:
> > > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > > provided as a service via reference in ACPI to the PCCT table entry
> > > allowing this to find the mailbox device - which is a platform
> > > device drivers/mailbox/pcc.c).
> > > Because it's all spec defined content in the mailbox messages, we don't
> > > have the more flexible (and newer I think) 'register' via operation region
> > > stuff in AML.
> > >
> > > A wrinkle though.  The mailbox data is mapped into this driver via
> > > an acpi_os_ioremap() call.
> > >
> > > So I'm thinking we don't have a strong reason for a platform device
> > > other than 'similarity' to other examples.  Never the strongest reason!
> > >
> > > We'll explore alternatives and see what they end up looking like.
> > >
> > > Jonathan
> > >
> >
> > Greg,
> >
> > I'm struggling a little to figure out how you envision the virtual bus
> > working here.  So before we spend too much time implementing the wrong thing
> > as it feels non trivial, let me check my understanding.
> >
> > Would this mean registering a ras2 bus via subsys_virtual_register().
> > (Similar to done for memory tiers)
>
> It should show up under /sys/devices/virtual/ is what I mean.
>
> > On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> > are one per independent feature). Each feature has its own mailbox sub
> > channel (via a reference to the PCC mailbox devices .
> > Typically you have one of these per feature type per numa node, but
> > that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> > so that the RAS2 edac feature drivers can find this later and bind to it to
> > register with edac etc.
> >
> > So spinning up a full new bus, to support this?  I'm not against that.
>
> No, again, see how the stuff that shows up in /sys/devices/virtual
> works, that should be much simpler.
>
> But really, as this is a "bus", just make a new one.  I don't understand
> why ACPI isn't creating your devices for you, as this is ACPI code,
> perhaps just fix that up instead?  That would make much more sense to
> me...

Because it is a data-only table, not AML.

It looks to me like this could be an auxiliary device, similar to the
Intel VSEC driver: see intel_vsec_add_aux() etc.

Cheers, Rafael
Jonathan Cameron Oct. 15, 2024, 1:34 p.m. UTC | #10
On Tue, 15 Oct 2024 12:17:25 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:
> > On Tue, 15 Oct 2024 10:10:25 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Mon, 14 Oct 2024 20:06:40 +0200
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >   
> > > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > > <Jonathan.Cameron@huawei.com> wrote:  
> > > > >
> > > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >    
> > > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:    
> > > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:    
> > > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > > >    
> > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > >
> > > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > > ---
> > > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > > >
> > > > > > > > >  struct platform_driver {
> > > > > > > > >         int (*probe)(struct platform_device *);    
> > > > > > > >
> > > > > > > > +CC Greg KH and Rafael.
> > > > > > > >
> > > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > > of view I think.    
> > > > > > >
> > > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > > to a patch that uses this?    
> > > > > >
> > > > > > Ah, is it this one:
> > > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > > ?
> > > > > >
> > > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > > device layer for something like this.    
> > > > >
> > > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > > Kind of similar(ish) to say IORT.    
> > > > 
> > > > Good question.
> > > > 
> > > > I guess it depends on whether or not there are any registers to access
> > > > or AML to interact with.  If so, I think that a platform device makes
> > > > sense.  
> > > 
> > > Unfortunately still a gray area I think.
> > > 
> > > This does access mailbox memory addresses, but they are provided
> > > by an existing platform device, so maybe platform device for this
> > > device is still inappropriate :(
> > > 
> > > What this uses is:
> > > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > > provided as a service via reference in ACPI to the PCCT table entry
> > > allowing this to find the mailbox device - which is a platform
> > > device drivers/mailbox/pcc.c).
> > > Because it's all spec defined content in the mailbox messages, we don't
> > > have the more flexible (and newer I think) 'register' via operation region
> > > stuff in AML.
> > > 
> > > A wrinkle though.  The mailbox data is mapped into this driver via
> > > an acpi_os_ioremap() call.  
> > > 
> > > So I'm thinking we don't have a strong reason for a platform device
> > > other than 'similarity' to other examples.  Never the strongest reason!
> > > 
> > > We'll explore alternatives and see what they end up looking like.
> > > 
> > > Jonathan
> > >   
> > 
> > Greg,
> > 
> > I'm struggling a little to figure out how you envision the virtual bus
> > working here.  So before we spend too much time implementing the wrong thing
> > as it feels non trivial, let me check my understanding.
> > 
> > Would this mean registering a ras2 bus via subsys_virtual_register().
> > (Similar to done for memory tiers)  
> 
> It should show up under /sys/devices/virtual/ is what I mean.
> 
> > On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> > are one per independent feature). Each feature has its own mailbox sub
> > channel (via a reference to the PCC mailbox devices .
> > Typically you have one of these per feature type per numa node, but
> > that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> > so that the RAS2 edac feature drivers can find this later and bind to it to
> > register with edac etc.
> > 
> > So spinning up a full new bus, to support this?  I'm not against that.  
> 
> No, again, see how the stuff that shows up in /sys/devices/virtual
> works, that should be much simpler.
> 
> But really, as this is a "bus", just make a new one.  I don't understand
> why ACPI isn't creating your devices for you, as this is ACPI code,
> perhaps just fix that up instead?  

Filling in that registration gap was the purpose of the next patch.

In all the similar cases that I can find where a static ACPI table is parsed
(maybe Rafael can point at another) one of two things happen:
1) The table is parsed and platform devices are created for the entries found
  (IORT etc) to which drivers later bind.
2) There is a 'fake' device in DSDT. E.g. ACPI0012 for NFIT, ACPI0017 for CEDT
(the CXL table) that are there to trigger enumeration of the static table and
those are done via device on the acpi bus.  That allows late enumeration and
registration of devices with the appropriate classes etc.

We'll explore the options but right now I'm thinking a new bus/acpi_ras2 is probably
the way to go.

Jonathan




> That would make much more sense to
> me...
> 
> thanks,
> 
> greg k-h
>
Rafael J. Wysocki Oct. 15, 2024, 1:37 p.m. UTC | #11
On Tue, Oct 15, 2024 at 3:34 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Oct 2024 12:17:25 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:
> > > On Tue, 15 Oct 2024 10:10:25 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >
> > > > On Mon, 14 Oct 2024 20:06:40 +0200
> > > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > > >
> > > > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > > > <Jonathan.Cameron@huawei.com> wrote:
> > > > > >
> > > > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:
> > > > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:
> > > > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > > > >
> > > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > >
> > > > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > > > ---
> > > > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > > > >
> > > > > > > > > >  struct platform_driver {
> > > > > > > > > >         int (*probe)(struct platform_device *);
> > > > > > > > >
> > > > > > > > > +CC Greg KH and Rafael.
> > > > > > > > >
> > > > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > > > of view I think.
> > > > > > > >
> > > > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > > > to a patch that uses this?
> > > > > > >
> > > > > > > Ah, is it this one:
> > > > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > > > ?
> > > > > > >
> > > > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > > > device layer for something like this.
> > > > > >
> > > > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > > > Kind of similar(ish) to say IORT.
> > > > >
> > > > > Good question.
> > > > >
> > > > > I guess it depends on whether or not there are any registers to access
> > > > > or AML to interact with.  If so, I think that a platform device makes
> > > > > sense.
> > > >
> > > > Unfortunately still a gray area I think.
> > > >
> > > > This does access mailbox memory addresses, but they are provided
> > > > by an existing platform device, so maybe platform device for this
> > > > device is still inappropriate :(
> > > >
> > > > What this uses is:
> > > > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > > > provided as a service via reference in ACPI to the PCCT table entry
> > > > allowing this to find the mailbox device - which is a platform
> > > > device drivers/mailbox/pcc.c).
> > > > Because it's all spec defined content in the mailbox messages, we don't
> > > > have the more flexible (and newer I think) 'register' via operation region
> > > > stuff in AML.
> > > >
> > > > A wrinkle though.  The mailbox data is mapped into this driver via
> > > > an acpi_os_ioremap() call.
> > > >
> > > > So I'm thinking we don't have a strong reason for a platform device
> > > > other than 'similarity' to other examples.  Never the strongest reason!
> > > >
> > > > We'll explore alternatives and see what they end up looking like.
> > > >
> > > > Jonathan
> > > >
> > >
> > > Greg,
> > >
> > > I'm struggling a little to figure out how you envision the virtual bus
> > > working here.  So before we spend too much time implementing the wrong thing
> > > as it feels non trivial, let me check my understanding.
> > >
> > > Would this mean registering a ras2 bus via subsys_virtual_register().
> > > (Similar to done for memory tiers)
> >
> > It should show up under /sys/devices/virtual/ is what I mean.
> >
> > > On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> > > are one per independent feature). Each feature has its own mailbox sub
> > > channel (via a reference to the PCC mailbox devices .
> > > Typically you have one of these per feature type per numa node, but
> > > that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> > > so that the RAS2 edac feature drivers can find this later and bind to it to
> > > register with edac etc.
> > >
> > > So spinning up a full new bus, to support this?  I'm not against that.
> >
> > No, again, see how the stuff that shows up in /sys/devices/virtual
> > works, that should be much simpler.
> >
> > But really, as this is a "bus", just make a new one.  I don't understand
> > why ACPI isn't creating your devices for you, as this is ACPI code,
> > perhaps just fix that up instead?
>
> Filling in that registration gap was the purpose of the next patch.
>
> In all the similar cases that I can find where a static ACPI table is parsed
> (maybe Rafael can point at another) one of two things happen:
> 1) The table is parsed and platform devices are created for the entries found
>   (IORT etc) to which drivers later bind.
> 2) There is a 'fake' device in DSDT. E.g. ACPI0012 for NFIT, ACPI0017 for CEDT
> (the CXL table) that are there to trigger enumeration of the static table and
> those are done via device on the acpi bus.  That allows late enumeration and
> registration of devices with the appropriate classes etc.
>
> We'll explore the options but right now I'm thinking a new bus/acpi_ras2 is probably
> the way to go.

Have you looked at auxiliary devices?
Jonathan Cameron Oct. 15, 2024, 2:19 p.m. UTC | #12
On Tue, 15 Oct 2024 15:32:28 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Tue, Oct 15, 2024 at 12:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:  
> > > On Tue, 15 Oct 2024 10:10:25 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >  
> > > > On Mon, 14 Oct 2024 20:06:40 +0200
> > > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > > >  
> > > > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > > > <Jonathan.Cameron@huawei.com> wrote:  
> > > > > >
> > > > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >  
> > > > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:  
> > > > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:  
> > > > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > > > >  
> > > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > >
> > > > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > > > ---
> > > > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > > > >
> > > > > > > > > >  struct platform_driver {
> > > > > > > > > >         int (*probe)(struct platform_device *);  
> > > > > > > > >
> > > > > > > > > +CC Greg KH and Rafael.
> > > > > > > > >
> > > > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > > > of view I think.  
> > > > > > > >
> > > > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > > > to a patch that uses this?  
> > > > > > >
> > > > > > > Ah, is it this one:
> > > > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > > > ?
> > > > > > >
> > > > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > > > device layer for something like this.  
> > > > > >
> > > > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > > > Kind of similar(ish) to say IORT.  
> > > > >
> > > > > Good question.
> > > > >
> > > > > I guess it depends on whether or not there are any registers to access
> > > > > or AML to interact with.  If so, I think that a platform device makes
> > > > > sense.  
> > > >
> > > > Unfortunately still a gray area I think.
> > > >
> > > > This does access mailbox memory addresses, but they are provided
> > > > by an existing platform device, so maybe platform device for this
> > > > device is still inappropriate :(
> > > >
> > > > What this uses is:
> > > > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > > > provided as a service via reference in ACPI to the PCCT table entry
> > > > allowing this to find the mailbox device - which is a platform
> > > > device drivers/mailbox/pcc.c).
> > > > Because it's all spec defined content in the mailbox messages, we don't
> > > > have the more flexible (and newer I think) 'register' via operation region
> > > > stuff in AML.
> > > >
> > > > A wrinkle though.  The mailbox data is mapped into this driver via
> > > > an acpi_os_ioremap() call.
> > > >
> > > > So I'm thinking we don't have a strong reason for a platform device
> > > > other than 'similarity' to other examples.  Never the strongest reason!
> > > >
> > > > We'll explore alternatives and see what they end up looking like.
> > > >
> > > > Jonathan
> > > >  
> > >
> > > Greg,
> > >
> > > I'm struggling a little to figure out how you envision the virtual bus
> > > working here.  So before we spend too much time implementing the wrong thing
> > > as it feels non trivial, let me check my understanding.
> > >
> > > Would this mean registering a ras2 bus via subsys_virtual_register().
> > > (Similar to done for memory tiers)  
> >
> > It should show up under /sys/devices/virtual/ is what I mean.
> >  
> > > On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> > > are one per independent feature). Each feature has its own mailbox sub
> > > channel (via a reference to the PCC mailbox devices .
> > > Typically you have one of these per feature type per numa node, but
> > > that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> > > so that the RAS2 edac feature drivers can find this later and bind to it to
> > > register with edac etc.
> > >
> > > So spinning up a full new bus, to support this?  I'm not against that.  
> >
> > No, again, see how the stuff that shows up in /sys/devices/virtual
> > works, that should be much simpler.
> >
> > But really, as this is a "bus", just make a new one.  I don't understand
> > why ACPI isn't creating your devices for you, as this is ACPI code,
> > perhaps just fix that up instead?  That would make much more sense to
> > me...  
> 
> Because it is a data-only table, not AML.
> 
> It looks to me like this could be an auxiliary device, similar to the
> Intel VSEC driver: see intel_vsec_add_aux() etc.
> 

That was in the other branch of the thread abbreviated as auxbus.
My concern with that approach is we have no parent device and the
auxiliary bus is always described as being for sub parts of a
compound device. In the intel_vsec case there is always a parent
pci device or platform device.

I don't think there is any functional requirement for a real parent,
it just feels like abuse given the stated purpose of auxiliary bus.
Greg, auxiliary bus or separate acpi_ras2 bus feel better to you?

We'd need to parent it off something to avoid the check in
auxiliary_device_init() + all devices should have a parent anyway.

I was thinking we could use the virtual device, but can only
get the kobj for that (there is no device), so nope - need a parent.
platform_device! (only kidding ;)

So my thinking is we are back with a new bus.

Jonathan



> Cheers, Rafael
>
Rafael J. Wysocki Oct. 15, 2024, 3:35 p.m. UTC | #13
On Tue, Oct 15, 2024 at 4:19 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Oct 2024 15:32:28 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Tue, Oct 15, 2024 at 12:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:
> > > > On Tue, 15 Oct 2024 10:10:25 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > >
> > > > > On Mon, 14 Oct 2024 20:06:40 +0200
> > > > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > > > >
> > > > > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > > > > <Jonathan.Cameron@huawei.com> wrote:
> > > > > > >
> > > > > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:
> > > > > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:
> > > > > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > > > > >
> > > > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > > >
> > > > > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > > > > >
> > > > > > > > > > >  struct platform_driver {
> > > > > > > > > > >         int (*probe)(struct platform_device *);
> > > > > > > > > >
> > > > > > > > > > +CC Greg KH and Rafael.
> > > > > > > > > >
> > > > > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > > > > of view I think.
> > > > > > > > >
> > > > > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > > > > to a patch that uses this?
> > > > > > > >
> > > > > > > > Ah, is it this one:
> > > > > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > > > > ?
> > > > > > > >
> > > > > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > > > > device layer for something like this.
> > > > > > >
> > > > > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > > > > Kind of similar(ish) to say IORT.
> > > > > >
> > > > > > Good question.
> > > > > >
> > > > > > I guess it depends on whether or not there are any registers to access
> > > > > > or AML to interact with.  If so, I think that a platform device makes
> > > > > > sense.
> > > > >
> > > > > Unfortunately still a gray area I think.
> > > > >
> > > > > This does access mailbox memory addresses, but they are provided
> > > > > by an existing platform device, so maybe platform device for this
> > > > > device is still inappropriate :(
> > > > >
> > > > > What this uses is:
> > > > > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > > > > provided as a service via reference in ACPI to the PCCT table entry
> > > > > allowing this to find the mailbox device - which is a platform
> > > > > device drivers/mailbox/pcc.c).
> > > > > Because it's all spec defined content in the mailbox messages, we don't
> > > > > have the more flexible (and newer I think) 'register' via operation region
> > > > > stuff in AML.
> > > > >
> > > > > A wrinkle though.  The mailbox data is mapped into this driver via
> > > > > an acpi_os_ioremap() call.
> > > > >
> > > > > So I'm thinking we don't have a strong reason for a platform device
> > > > > other than 'similarity' to other examples.  Never the strongest reason!
> > > > >
> > > > > We'll explore alternatives and see what they end up looking like.
> > > > >
> > > > > Jonathan
> > > > >
> > > >
> > > > Greg,
> > > >
> > > > I'm struggling a little to figure out how you envision the virtual bus
> > > > working here.  So before we spend too much time implementing the wrong thing
> > > > as it feels non trivial, let me check my understanding.
> > > >
> > > > Would this mean registering a ras2 bus via subsys_virtual_register().
> > > > (Similar to done for memory tiers)
> > >
> > > It should show up under /sys/devices/virtual/ is what I mean.
> > >
> > > > On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> > > > are one per independent feature). Each feature has its own mailbox sub
> > > > channel (via a reference to the PCC mailbox devices .
> > > > Typically you have one of these per feature type per numa node, but
> > > > that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> > > > so that the RAS2 edac feature drivers can find this later and bind to it to
> > > > register with edac etc.
> > > >
> > > > So spinning up a full new bus, to support this?  I'm not against that.
> > >
> > > No, again, see how the stuff that shows up in /sys/devices/virtual
> > > works, that should be much simpler.
> > >
> > > But really, as this is a "bus", just make a new one.  I don't understand
> > > why ACPI isn't creating your devices for you, as this is ACPI code,
> > > perhaps just fix that up instead?  That would make much more sense to
> > > me...
> >
> > Because it is a data-only table, not AML.
> >
> > It looks to me like this could be an auxiliary device, similar to the
> > Intel VSEC driver: see intel_vsec_add_aux() etc.
> >
>
> That was in the other branch of the thread abbreviated as auxbus.
> My concern with that approach is we have no parent device and the
> auxiliary bus is always described as being for sub parts of a
> compound device. In the intel_vsec case there is always a parent
> pci device or platform device.
>
> I don't think there is any functional requirement for a real parent,
> it just feels like abuse given the stated purpose of auxiliary bus.
> Greg, auxiliary bus or separate acpi_ras2 bus feel better to you?
>
> We'd need to parent it off something to avoid the check in
> auxiliary_device_init() + all devices should have a parent anyway.

Wouldn't that be the platform device providing the mailbox memory
addresses mentioned in one of the previous messages?
Jonathan Cameron Oct. 16, 2024, 9 a.m. UTC | #14
On Tue, 15 Oct 2024 17:35:31 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Tue, Oct 15, 2024 at 4:19 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Tue, 15 Oct 2024 15:32:28 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >  
> > > On Tue, Oct 15, 2024 at 12:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:  
> > > >
> > > > On Tue, Oct 15, 2024 at 10:40:54AM +0100, Jonathan Cameron wrote:  
> > > > > On Tue, 15 Oct 2024 10:10:25 +0100
> > > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > > >  
> > > > > > On Mon, 14 Oct 2024 20:06:40 +0200
> > > > > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > > > > >  
> > > > > > > On Mon, Oct 14, 2024 at 7:17 PM Jonathan Cameron
> > > > > > > <Jonathan.Cameron@huawei.com> wrote:  
> > > > > > > >
> > > > > > > > On Mon, 14 Oct 2024 18:04:37 +0200
> > > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >  
> > > > > > > > > On Mon, Oct 14, 2024 at 06:00:51PM +0200, Greg KH wrote:  
> > > > > > > > > > On Mon, Oct 14, 2024 at 04:43:39PM +0100, Jonathan Cameron wrote:  
> > > > > > > > > > > On Wed, 9 Oct 2024 13:41:13 +0100
> > > > > > > > > > > <shiju.jose@huawei.com> wrote:
> > > > > > > > > > >  
> > > > > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Add __free() based cleanup function for platform_device_put().
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > > > > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  include/linux/platform_device.h | 1 +
> > > > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > > > > > > > > > > > index d422db6eec63..606533b88f44 100644
> > > > > > > > > > > > --- a/include/linux/platform_device.h
> > > > > > > > > > > > +++ b/include/linux/platform_device.h
> > > > > > > > > > > > @@ -232,6 +232,7 @@ extern int platform_device_add_data(struct platform_device *pdev,
> > > > > > > > > > > >  extern int platform_device_add(struct platform_device *pdev);
> > > > > > > > > > > >  extern void platform_device_del(struct platform_device *pdev);
> > > > > > > > > > > >  extern void platform_device_put(struct platform_device *pdev);
> > > > > > > > > > > > +DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
> > > > > > > > > > > >
> > > > > > > > > > > >  struct platform_driver {
> > > > > > > > > > > >         int (*probe)(struct platform_device *);  
> > > > > > > > > > >
> > > > > > > > > > > +CC Greg KH and Rafael.
> > > > > > > > > > >
> > > > > > > > > > > Makes sure to include them on v14 as this needs review from a driver core point
> > > > > > > > > > > of view I think.  
> > > > > > > > > >
> > > > > > > > > > Why is this needed for a platform device?  This feels like you will have
> > > > > > > > > > to do more work to "keep" the reference on the normal path than you to
> > > > > > > > > > today to release the reference on the error path, right?  Have a pointer
> > > > > > > > > > to a patch that uses this?  
> > > > > > > > >
> > > > > > > > > Ah, is it this one:
> > > > > > > > >       https://lore.kernel.org/all/20241014164955.00003439@Huawei.com/
> > > > > > > > > ?
> > > > > > > > >
> > > > > > > > > If so, no, that's an abuse of a platform device, don't do that, make a
> > > > > > > > > REAL device on the bus that this device lives on.  If it doesn't live on
> > > > > > > > > a real bus, then put it on the virtual bus but do NOT abuse the platform
> > > > > > > > > device layer for something like this.  
> > > > > > > >
> > > > > > > > Ok.  Probably virtual bus it is then.  Rafael, what do you think makes sense
> > > > > > > > for a 'feature' that is described only by an ACPI table (here RAS2)?
> > > > > > > > Kind of similar(ish) to say IORT.  
> > > > > > >
> > > > > > > Good question.
> > > > > > >
> > > > > > > I guess it depends on whether or not there are any registers to access
> > > > > > > or AML to interact with.  If so, I think that a platform device makes
> > > > > > > sense.  
> > > > > >
> > > > > > Unfortunately still a gray area I think.
> > > > > >
> > > > > > This does access mailbox memory addresses, but they are provided
> > > > > > by an existing platform device, so maybe platform device for this
> > > > > > device is still inappropriate :(
> > > > > >
> > > > > > What this uses is:
> > > > > > PCC channel (mailbox in memory + doorbells, etc but that is indirectly
> > > > > > provided as a service via reference in ACPI to the PCCT table entry
> > > > > > allowing this to find the mailbox device - which is a platform
> > > > > > device drivers/mailbox/pcc.c).
> > > > > > Because it's all spec defined content in the mailbox messages, we don't
> > > > > > have the more flexible (and newer I think) 'register' via operation region
> > > > > > stuff in AML.
> > > > > >
> > > > > > A wrinkle though.  The mailbox data is mapped into this driver via
> > > > > > an acpi_os_ioremap() call.
> > > > > >
> > > > > > So I'm thinking we don't have a strong reason for a platform device
> > > > > > other than 'similarity' to other examples.  Never the strongest reason!
> > > > > >
> > > > > > We'll explore alternatives and see what they end up looking like.
> > > > > >
> > > > > > Jonathan
> > > > > >  
> > > > >
> > > > > Greg,
> > > > >
> > > > > I'm struggling a little to figure out how you envision the virtual bus
> > > > > working here.  So before we spend too much time implementing the wrong thing
> > > > > as it feels non trivial, let me check my understanding.
> > > > >
> > > > > Would this mean registering a ras2 bus via subsys_virtual_register().
> > > > > (Similar to done for memory tiers)  
> > > >
> > > > It should show up under /sys/devices/virtual/ is what I mean.
> > > >  
> > > > > On that we'd then add all the devices: one per RAS2 PCC descriptor (these
> > > > > are one per independent feature). Each feature has its own mailbox sub
> > > > > channel (via a reference to the PCC mailbox devices .
> > > > > Typically you have one of these per feature type per numa node, but
> > > > > that isn't guaranteed.  That will then need wiring up with bus->probe() etc
> > > > > so that the RAS2 edac feature drivers can find this later and bind to it to
> > > > > register with edac etc.
> > > > >
> > > > > So spinning up a full new bus, to support this?  I'm not against that.  
> > > >
> > > > No, again, see how the stuff that shows up in /sys/devices/virtual
> > > > works, that should be much simpler.
> > > >
> > > > But really, as this is a "bus", just make a new one.  I don't understand
> > > > why ACPI isn't creating your devices for you, as this is ACPI code,
> > > > perhaps just fix that up instead?  That would make much more sense to
> > > > me...  
> > >
> > > Because it is a data-only table, not AML.
> > >
> > > It looks to me like this could be an auxiliary device, similar to the
> > > Intel VSEC driver: see intel_vsec_add_aux() etc.
> > >  
> >
> > That was in the other branch of the thread abbreviated as auxbus.
> > My concern with that approach is we have no parent device and the
> > auxiliary bus is always described as being for sub parts of a
> > compound device. In the intel_vsec case there is always a parent
> > pci device or platform device.
> >
> > I don't think there is any functional requirement for a real parent,
> > it just feels like abuse given the stated purpose of auxiliary bus.
> > Greg, auxiliary bus or separate acpi_ras2 bus feel better to you?
> >
> > We'd need to parent it off something to avoid the check in
> > auxiliary_device_init() + all devices should have a parent anyway.  
> 
> Wouldn't that be the platform device providing the mailbox memory
> addresses mentioned in one of the previous messages?

Added Sudeep and Jassi given this feels like we'd need their input
to consider doing this.

Hmm. Probably works, though it will be a little inelegant as we'll have
a discovery path unrelated to the mailbox provider discovery path
that then instantiates children of the mailbox device. This is just
one consumer of the mailbox device. It feels not too bad for this particular
combination at all because RAS2 entries don't have any other resources
so unlike many PCC channel users we wouldn't be introduce devices with
wider scope than the mailbox parent device (note I think there is only
device for all PCC in the system - each actual mailbox is a PCC subspace).

1) Mailbox path:
	acpi_pcc_probe() in drivers/mailbox/pcc.c via postcore_init_call()
 	Checks there are PCC channels in PCCT ACPI table.
	Instantiates a platform device and binds that via
	platform_create_bundle()
	Probes the available channels and stashes all the info
	about them in arrays associated with that platform device.
	Calls mbox_controller_register() which sounds like as
	class registration but isn't. That just puts it on
	LIST_HEAD(mbox_cons) so that it can be searched for
	by consumers of this channel.
2) RAS2 parsing (tweaked version of patch 13)
	acpi_ras2_probe() currently via a late_initcall()
	Checks for RAS2 table and for each RAS2 PCC descriptor and
	gets a channel from the appropriate mailbox via
	pcc_mbox_request_channel() giving us a struct pcc_mbox_chan.
	Then we would need to get from there to the platform device
	that represents all the mailboxes.
	pcc_mbox->mchan->mbox_controller->dev is it I think.
	Finally we parent our new device off that.

What do people think of this vs option of spinning a new bus/acpi_ras2
and no parent relationship between a ras2 feature entry the PCC
platform device - just a client of mailbox relationship
(effectively what is in patch 13, but with devices on a new bus rather
than as platform_device)
https://lore.kernel.org/all/20241009124120.1124-14-shiju.jose@huawei.com/
is that patch.

Jonathan
diff mbox series

Patch

diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index d422db6eec63..606533b88f44 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -232,6 +232,7 @@  extern int platform_device_add_data(struct platform_device *pdev,
 extern int platform_device_add(struct platform_device *pdev);
 extern void platform_device_del(struct platform_device *pdev);
 extern void platform_device_put(struct platform_device *pdev);
+DEFINE_FREE(platform_device_put, struct platform_device *, if (_T) platform_device_put(_T))
 
 struct platform_driver {
 	int (*probe)(struct platform_device *);