Introducing a callback in struct bus_type so that a subsystem
can hook up the getters directly. This approach avoids exposing
random getters in any subsystems APIs.
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
include/linux/device/bus.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..b18658bce2c3819fc1cbeb38fb98391d56ec3317 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -48,6 +48,7 @@ struct fwnode_handle;
* will never get called until they do.
* @remove: Called when a device removed from this bus.
* @shutdown: Called at shut-down time to quiesce the device.
+ * @irq_get_affinity: Get IRQ affinity mask for the device on this bus.
*
* @online: Called to put the device back online (after offlining it).
* @offline: Called to put the device offline for hot-removal. May fail.
@@ -87,6 +88,8 @@ struct bus_type {
void (*sync_state)(struct device *dev);
void (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
+ const struct cpumask *(*irq_get_affinity)(struct device *dev,
+ unsigned int irq_vec);
int (*online)(struct device *dev);
int (*offline)(struct device *dev);
--
2.47.0On 12/11/2024 13:26, Daniel Wagner wrote: > Introducing a callback in struct bus_type so that a subsystem > can hook up the getters directly. This approach avoids exposing > random getters in any subsystems APIs. > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Daniel Wagner <wagi@kernel.org> > --- > include/linux/device/bus.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..b18658bce2c3819fc1cbeb38fb98391d56ec3317 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -48,6 +48,7 @@ struct fwnode_handle; > * will never get called until they do. > * @remove: Called when a device removed from this bus. My impression is that this would be better suited to "struct device_driver", but I assume that there is a good reason to add to "struct bus_type".
On Wed, Nov 13, 2024 at 10:16:32AM +0000, John Garry wrote:
> On 12/11/2024 13:26, Daniel Wagner wrote:
> > Introducing a callback in struct bus_type so that a subsystem
> > can hook up the getters directly. This approach avoids exposing
> > random getters in any subsystems APIs.
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Daniel Wagner <wagi@kernel.org>
> > ---
> > include/linux/device/bus.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> > index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..b18658bce2c3819fc1cbeb38fb98391d56ec3317 100644
> > --- a/include/linux/device/bus.h
> > +++ b/include/linux/device/bus.h
> > @@ -48,6 +48,7 @@ struct fwnode_handle;
> > * will never get called until they do.
> > * @remove: Called when a device removed from this bus.
>
> My impression is that this would be better suited to "struct device_driver",
> but I assume that there is a good reason to add to "struct bus_type".
I think the main reason to put it here is that most of the drivers are
happy with the getter on bus level and don't need special treatment. We
don't have to touch all the drivers to hookup a common getter, nor do we
have to install a default handler when the driver doesn't specify one.
Having the callback in struct bus_driver avoids this. Though Christoph
suggested it, so I can only guess.
But you bring up a good point, if we had also an irq_get_affinity
callback in struct device_driver it would be possible for the
hisi_sas v2 driver to provide a getter and blk_mq_hctx_map_queues could
do:
for (queue = 0; queue < qmap->nr_queues; queue++) {
if (dev->driver->irq_get_affinity)
mask = dev->driver->irq_get_affinity;
else if (dev->bus->irq_get_affinity)
mask = dev->bus->irq_get_affinity(dev, queue + offset);
if (!mask)
goto fallback;
for_each_cpu(cpu, mask)
qmap->mq_map[cpu] = qmap->queue_offset + queue;
}
and with this in place the open coded version in hisi_sas v2 can also be
replaced. If no one objects, I go ahead and add the callback to struct
device_driver.
On 13/11/2024 12:36, Daniel Wagner wrote:
>>> @@ -48,6 +48,7 @@ struct fwnode_handle;
>>> * will never get called until they do.
>>> * @remove: Called when a device removed from this bus.
>> My impression is that this would be better suited to "struct device_driver",
>> but I assume that there is a good reason to add to "struct bus_type".
> I think the main reason to put it here is that most of the drivers are
> happy with the getter on bus level and don't need special treatment. We
> don't have to touch all the drivers to hookup a common getter, nor do we
> have to install a default handler when the driver doesn't specify one.
> Having the callback in struct bus_driver avoids this. Though Christoph
> suggested it, so I can only guess.
>
> But you bring up a good point, if we had also an irq_get_affinity
> callback in struct device_driver it would be possible for the
> hisi_sas v2 driver to provide a getter and blk_mq_hctx_map_queues could
> do:
>
> for (queue = 0; queue < qmap->nr_queues; queue++) {
> if (dev->driver->irq_get_affinity)
> mask = dev->driver->irq_get_affinity;
> else if (dev->bus->irq_get_affinity)
> mask = dev->bus->irq_get_affinity(dev, queue + offset);
> if (!mask)
> goto fallback;
>
> for_each_cpu(cpu, mask)
> qmap->mq_map[cpu] = qmap->queue_offset + queue;
> }
>
> and with this in place the open coded version in hisi_sas v2 can also be
> replaced.
Yeah, I think that it could be plugged in like:
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 342d75f12051..5172af77a3f0 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = {
.name = DRV_NAME,
.of_match_table = sas_v2_of_match,
.acpi_match_table = ACPI_PTR(sas_v2_acpi_match),
+ .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask,
},
};
> If no one objects, I go ahead and add the callback to struct
> device_driver.
I'd wait for Christoph and Greg to both agree. I was just wondering why
we use bus_type.
On Wed, Nov 13, 2024 at 01:44:02PM +0000, John Garry wrote:
> On 13/11/2024 12:36, Daniel Wagner wrote:
> > > > @@ -48,6 +48,7 @@ struct fwnode_handle;
> > > > * will never get called until they do.
> > > > * @remove: Called when a device removed from this bus.
> > > My impression is that this would be better suited to "struct device_driver",
> > > but I assume that there is a good reason to add to "struct bus_type".
> > I think the main reason to put it here is that most of the drivers are
> > happy with the getter on bus level and don't need special treatment. We
> > don't have to touch all the drivers to hookup a common getter, nor do we
> > have to install a default handler when the driver doesn't specify one.
> > Having the callback in struct bus_driver avoids this. Though Christoph
> > suggested it, so I can only guess.
> >
> > But you bring up a good point, if we had also an irq_get_affinity
> > callback in struct device_driver it would be possible for the
> > hisi_sas v2 driver to provide a getter and blk_mq_hctx_map_queues could
> > do:
> >
> > for (queue = 0; queue < qmap->nr_queues; queue++) {
> > if (dev->driver->irq_get_affinity)
> > mask = dev->driver->irq_get_affinity;
> > else if (dev->bus->irq_get_affinity)
> > mask = dev->bus->irq_get_affinity(dev, queue + offset);
> > if (!mask)
> > goto fallback;
> >
> > for_each_cpu(cpu, mask)
> > qmap->mq_map[cpu] = qmap->queue_offset + queue;
> > }
> >
> > and with this in place the open coded version in hisi_sas v2 can also be
> > replaced.
>
> Yeah, I think that it could be plugged in like:
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> index 342d75f12051..5172af77a3f0 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = {
> .name = DRV_NAME,
> .of_match_table = sas_v2_of_match,
> .acpi_match_table = ACPI_PTR(sas_v2_acpi_match),
> + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask,
> },
> };
>
>
> > If no one objects, I go ahead and add the callback to struct
> > device_driver.
>
> I'd wait for Christoph and Greg to both agree. I was just wondering why we
> use bus_type.
bus types are good to set it at a bus level so you don't have to
explicitly set it at each-and-every-driver. Depends on what you want
this to be, if it is a "all drivers of this bus type will have the same
callback" then put it on the bus. otherwise if you are going to
mix/match on a same bus, then put it in the driver structure.
hope this helps,
greg k-h
On Wed, Nov 13, 2024 at 02:54:23PM +0100, Greg Kroah-Hartman wrote: > bus types are good to set it at a bus level so you don't have to > explicitly set it at each-and-every-driver. Depends on what you want > this to be, if it is a "all drivers of this bus type will have the same > callback" then put it on the bus. otherwise if you are going to > mix/match on a same bus, then put it in the driver structure. ... and that is exactly the case here. The driver itself has no business being involved.
On 13/11/2024 13:54, Greg Kroah-Hartman wrote:
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
>> b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
>> index 342d75f12051..5172af77a3f0 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
>> @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = {
>> .name = DRV_NAME,
>> .of_match_table = sas_v2_of_match,
>> .acpi_match_table = ACPI_PTR(sas_v2_acpi_match),
>> + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask,
>> },
>> };
>>
>>
>>> If no one objects, I go ahead and add the callback to struct
>>> device_driver.
>> I'd wait for Christoph and Greg to both agree. I was just wondering why we
>> use bus_type.
> bus types are good to set it at a bus level so you don't have to
> explicitly set it at each-and-every-driver. Depends on what you want
> this to be, if it is a "all drivers of this bus type will have the same
> callback" then put it on the bus. otherwise if you are going to
> mix/match on a same bus, then put it in the driver structure.
Understood, I think all drivers on same bus will use the same callback.
FWIW, most drivers of interest are pci drivers, so I thought that it
would simply be a change like this (for those drivers if we use struct
device_driver):
@@ -1442,6 +1455,7 @@ int __pci_register_driver(struct pci_driver
*drv, struct module *owner,
drv->driver.mod_name = mod_name;
drv->driver.groups = drv->groups;
drv->driver.dev_groups = drv->dev_groups;
+ drv->driver.irq_get_affinity = pci_device_irq_get_affinity;
Thanks,
John
On Wed, Nov 13, 2024 at 02:12:30PM +0000, John Garry wrote:
> On 13/11/2024 13:54, Greg Kroah-Hartman wrote:
> > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > > b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > > index 342d75f12051..5172af77a3f0 100644
> > > --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > > +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > > @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = {
> > > .name = DRV_NAME,
> > > .of_match_table = sas_v2_of_match,
> > > .acpi_match_table = ACPI_PTR(sas_v2_acpi_match),
> > > + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask,
> > > },
> > > };
> > >
> > >
> > > > If no one objects, I go ahead and add the callback to struct
> > > > device_driver.
> > > I'd wait for Christoph and Greg to both agree. I was just wondering why we
> > > use bus_type.
> > bus types are good to set it at a bus level so you don't have to
> > explicitly set it at each-and-every-driver. Depends on what you want
> > this to be, if it is a "all drivers of this bus type will have the same
> > callback" then put it on the bus. otherwise if you are going to
> > mix/match on a same bus, then put it in the driver structure.
>
> Understood, I think all drivers on same bus will use the same callback.
>
> FWIW, most drivers of interest are pci drivers, so I thought that it would
> simply be a change like this (for those drivers if we use struct
> device_driver):
>
> @@ -1442,6 +1455,7 @@ int __pci_register_driver(struct pci_driver
> *drv, struct module *owner,
> drv->driver.mod_name = mod_name;
> drv->driver.groups = drv->groups;
> drv->driver.dev_groups = drv->dev_groups;
> + drv->driver.irq_get_affinity = pci_device_irq_get_affinity;
Yes, you can do that too. But now you have a pointer-per-driver instead
of just one-per-bus. It's not a big deal, but again if this is always
going to be the same for everything on a bus, make it a bus pointer
please.
thanks,
greg k-h
On 11/12/24 14:26, Daniel Wagner wrote: > Introducing a callback in struct bus_type so that a subsystem > can hook up the getters directly. This approach avoids exposing > random getters in any subsystems APIs. > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Daniel Wagner <wagi@kernel.org> > --- > include/linux/device/bus.h | 3 +++ > 1 file changed, 3 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Han nes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
© 2016 - 2024 Red Hat, Inc.