| Message ID | 20230119013602.607466-2-tianfei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | add FPGA hotplug manager driver | expand |
On Wed, Jan 18, 2023 at 08:35:51PM -0500, Tianfei Zhang wrote: > To reprogram an PCIe-based FPGA card, a new image is > burned into FLASH on the card and then the card BMC is > triggered to reboot the card and load the new image. > > Two new operation callbacks are defined in hotplug_slot_ops > to trigger the reprogramming of an FPGA-based PCIe card: > > - available_images: Optional: available FPGA images > - image_load: Optional: trigger the FPGA to load a new image > > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/hotplug/pci_hotplug_core.c | 88 ++++++++++++++++++++++++++ > include/linux/pci_hotplug.h | 5 ++ > 2 files changed, 93 insertions(+) > > diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c > index 058d5937d8a9..2b14b6513a03 100644 > --- a/drivers/pci/hotplug/pci_hotplug_core.c > +++ b/drivers/pci/hotplug/pci_hotplug_core.c > @@ -231,6 +231,52 @@ static struct pci_slot_attribute hotplug_slot_attr_test = { > .store = test_write_file > }; > > +static ssize_t available_images_read_file(struct pci_slot *pci_slot, char *buf) > +{ > + struct hotplug_slot *slot = pci_slot->hotplug; > + ssize_t count = 0; > + > + if (!try_module_get(slot->owner)) > + return -ENODEV; > + > + if (slot->ops->available_images(slot, buf)) > + count = slot->ops->available_images(slot, buf); > + > + module_put(slot->owner); > + > + return count; > +} > + > +static struct pci_slot_attribute hotplug_slot_attr_available_images = { > + .attr = { .name = "available_images", .mode = 0444 }, > + .show = available_images_read_file, If you name things properly, you can use the correct macros and not have to open-code any of this :( > +static ssize_t image_load_write_file(struct pci_slot *pci_slot, > + const char *buf, size_t count) > +{ > + struct hotplug_slot *slot = pci_slot->hotplug; > + int retval = 0; > + > + if (!try_module_get(slot->owner)) > + return -ENODEV; > + > + if (slot->ops->image_load) > + retval = slot->ops->image_load(slot, buf); > + > + module_put(slot->owner); > + > + if (retval) > + return retval; > + > + return count; > +} > + > +static struct pci_slot_attribute hotplug_slot_attr_image_load = { > + .attr = { .name = "image_load", .mode = 0644 }, > + .store = image_load_write_file, Same here, don't open-code this. > +}; > + > static bool has_power_file(struct pci_slot *pci_slot) > { > struct hotplug_slot *slot = pci_slot->hotplug; > @@ -289,6 +335,20 @@ static bool has_test_file(struct pci_slot *pci_slot) > return false; > } > > +static bool has_available_images_file(struct pci_slot *pci_slot) > +{ > + struct hotplug_slot *slot = pci_slot->hotplug; > + > + return slot && slot->ops && slot->ops->available_images; > +} > + > +static bool has_image_load_file(struct pci_slot *pci_slot) > +{ > + struct hotplug_slot *slot = pci_slot->hotplug; > + > + return slot && slot->ops && slot->ops->image_load; > +} > + > static int fs_add_slot(struct pci_slot *pci_slot) > { > int retval = 0; > @@ -331,8 +391,30 @@ static int fs_add_slot(struct pci_slot *pci_slot) > goto exit_test; > } > > + if (has_available_images_file(pci_slot)) { > + retval = sysfs_create_file(&pci_slot->kobj, > + &hotplug_slot_attr_available_images.attr); > + if (retval) > + goto exit_available_images; > + } > + > + if (has_image_load_file(pci_slot)) { > + retval = sysfs_create_file(&pci_slot->kobj, > + &hotplug_slot_attr_image_load.attr); > + if (retval) > + goto exit_image_load; > + } > + > goto exit; > > +exit_image_load: > + if (has_adapter_file(pci_slot)) > + sysfs_remove_file(&pci_slot->kobj, > + &hotplug_slot_attr_available_images.attr); > +exit_available_images: > + if (has_adapter_file(pci_slot)) > + sysfs_remove_file(&pci_slot->kobj, > + &hotplug_slot_attr_test.attr); > exit_test: > if (has_adapter_file(pci_slot)) > sysfs_remove_file(&pci_slot->kobj, > @@ -372,6 +454,12 @@ static void fs_remove_slot(struct pci_slot *pci_slot) > if (has_test_file(pci_slot)) > sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_test.attr); > > + if (has_available_images_file(pci_slot)) > + sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_available_images.attr); > + > + if (has_image_load_file(pci_slot)) > + sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_image_load.attr); > + Ick no, please just make this an attribute group that properly shows or does not show, the attribute when created. Do not manually add individual sysfs files. Yes, I know the existing code does this, so it's not really your fault, but let's not persist in making this code even messier. Convert to a group first and then your new files will be added automagically without having to care about anything here at all. thanks, greg k-h
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index 058d5937d8a9..2b14b6513a03 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -231,6 +231,52 @@ static struct pci_slot_attribute hotplug_slot_attr_test = { .store = test_write_file }; +static ssize_t available_images_read_file(struct pci_slot *pci_slot, char *buf) +{ + struct hotplug_slot *slot = pci_slot->hotplug; + ssize_t count = 0; + + if (!try_module_get(slot->owner)) + return -ENODEV; + + if (slot->ops->available_images(slot, buf)) + count = slot->ops->available_images(slot, buf); + + module_put(slot->owner); + + return count; +} + +static struct pci_slot_attribute hotplug_slot_attr_available_images = { + .attr = { .name = "available_images", .mode = 0444 }, + .show = available_images_read_file, +}; + +static ssize_t image_load_write_file(struct pci_slot *pci_slot, + const char *buf, size_t count) +{ + struct hotplug_slot *slot = pci_slot->hotplug; + int retval = 0; + + if (!try_module_get(slot->owner)) + return -ENODEV; + + if (slot->ops->image_load) + retval = slot->ops->image_load(slot, buf); + + module_put(slot->owner); + + if (retval) + return retval; + + return count; +} + +static struct pci_slot_attribute hotplug_slot_attr_image_load = { + .attr = { .name = "image_load", .mode = 0644 }, + .store = image_load_write_file, +}; + static bool has_power_file(struct pci_slot *pci_slot) { struct hotplug_slot *slot = pci_slot->hotplug; @@ -289,6 +335,20 @@ static bool has_test_file(struct pci_slot *pci_slot) return false; } +static bool has_available_images_file(struct pci_slot *pci_slot) +{ + struct hotplug_slot *slot = pci_slot->hotplug; + + return slot && slot->ops && slot->ops->available_images; +} + +static bool has_image_load_file(struct pci_slot *pci_slot) +{ + struct hotplug_slot *slot = pci_slot->hotplug; + + return slot && slot->ops && slot->ops->image_load; +} + static int fs_add_slot(struct pci_slot *pci_slot) { int retval = 0; @@ -331,8 +391,30 @@ static int fs_add_slot(struct pci_slot *pci_slot) goto exit_test; } + if (has_available_images_file(pci_slot)) { + retval = sysfs_create_file(&pci_slot->kobj, + &hotplug_slot_attr_available_images.attr); + if (retval) + goto exit_available_images; + } + + if (has_image_load_file(pci_slot)) { + retval = sysfs_create_file(&pci_slot->kobj, + &hotplug_slot_attr_image_load.attr); + if (retval) + goto exit_image_load; + } + goto exit; +exit_image_load: + if (has_adapter_file(pci_slot)) + sysfs_remove_file(&pci_slot->kobj, + &hotplug_slot_attr_available_images.attr); +exit_available_images: + if (has_adapter_file(pci_slot)) + sysfs_remove_file(&pci_slot->kobj, + &hotplug_slot_attr_test.attr); exit_test: if (has_adapter_file(pci_slot)) sysfs_remove_file(&pci_slot->kobj, @@ -372,6 +454,12 @@ static void fs_remove_slot(struct pci_slot *pci_slot) if (has_test_file(pci_slot)) sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_test.attr); + if (has_available_images_file(pci_slot)) + sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_available_images.attr); + + if (has_image_load_file(pci_slot)) + sysfs_remove_file(&pci_slot->kobj, &hotplug_slot_attr_image_load.attr); + pci_hp_remove_module_link(pci_slot); } diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h index 3a10d6ec3ee7..b7f39c20ad8b 100644 --- a/include/linux/pci_hotplug.h +++ b/include/linux/pci_hotplug.h @@ -29,6 +29,9 @@ * @reset_slot: Optional interface to allow override of a bus reset for the * slot for cases where a secondary bus reset can result in spurious * hotplug events or where a slot can be reset independent of the bus. + * @available_images: Optional: called to get the available images for accelerator, + * like FPGA. + * @image_load: Optional: called to load a new image for accelerator like FPGA. * * The table of function pointers that is passed to the hotplug pci core by a * hotplug pci driver. These functions are called by the hotplug pci core when @@ -45,6 +48,8 @@ struct hotplug_slot_ops { int (*get_latch_status) (struct hotplug_slot *slot, u8 *value); int (*get_adapter_status) (struct hotplug_slot *slot, u8 *value); int (*reset_slot) (struct hotplug_slot *slot, bool probe); + int (*available_images) (struct hotplug_slot *slot, char *buf); + int (*image_load) (struct hotplug_slot *slot, const char *buf); }; /**