| Message ID | 20250213173925.200404-1-trintaeoitogc@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RESEND] PCI: cpci: remove unused fields | expand |
Le 13/02/2025 à 18:39, Guilherme Giacomo Simoes a écrit : > The `get_power()` and `set_power()` function pointers in the > `cpci_hp_controller ops` struct were declared but never implemented by > any driver. To improve code readability and reduce resource usage, > remove this pointers and replace with a `flags` field. > > Use the new `flags` field in `enable_slot()`, `disable_slot()`, and > `cpci_get_power_s atus()` to track the slot's power state using the > `SLOT_ENABLED` macro. > > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> > --- > drivers/pci/hotplug/cpci_hotplug.h | 3 +-- > drivers/pci/hotplug/cpci_hotplug_core.c | 21 +++++++-------------- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/hotplug/cpci_hotplug.h b/drivers/pci/hotplug/cpci_hotplug.h > index 03fa39ab0c88..c5cb12cad2b4 100644 > --- a/drivers/pci/hotplug/cpci_hotplug.h > +++ b/drivers/pci/hotplug/cpci_hotplug.h > @@ -44,8 +44,7 @@ struct cpci_hp_controller_ops { > int (*enable_irq)(void); > int (*disable_irq)(void); > int (*check_irq)(void *dev_id); > - u8 (*get_power)(struct slot *slot); > - int (*set_power)(struct slot *slot, int value); > + int flags; > }; > > struct cpci_hp_controller { > diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c > index d0559d2faf50..87a743c2a5f1 100644 > --- a/drivers/pci/hotplug/cpci_hotplug_core.c > +++ b/drivers/pci/hotplug/cpci_hotplug_core.c > @@ -27,6 +27,8 @@ > #define DRIVER_AUTHOR "Scott Murray <scottm@somanetworks.com>" > #define DRIVER_DESC "CompactPCI Hot Plug Core" > > +#define SLOT_ENABLED 0x00000001 > + > #define MY_NAME "cpci_hotplug" > > #define dbg(format, arg...) \ > @@ -71,13 +73,12 @@ static int > enable_slot(struct hotplug_slot *hotplug_slot) > { > struct slot *slot = to_slot(hotplug_slot); > - int retval = 0; > > dbg("%s - physical_slot = %s", __func__, slot_name(slot)); > > - if (controller->ops->set_power) > - retval = controller->ops->set_power(slot, 1); > - return retval; > + controller->ops->flags |= SLOT_ENABLED; > + > + return 0; > } > > static int > @@ -109,11 +110,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) > } > cpci_led_on(slot); > > - if (controller->ops->set_power) { > - retval = controller->ops->set_power(slot, 0); > - if (retval) > - goto disable_error; > - } > + controller->ops->flags &= ~SLOT_ENABLED; > > slot->adapter_status = 0; > > @@ -129,11 +126,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) > static u8 > cpci_get_power_status(struct slot *slot) > { > - u8 power = 1; > - > - if (controller->ops->get_power) > - power = controller->ops->get_power(slot); > - return power; > + return controller->ops->flags & SLOT_ENABLED; If neither get_power nor set_power where defined in any driver, then cpci_get_power_status() was always returning 1. IIUC, now it may return 1 or 0 depending of if enable_slot() or disable_slot() have been called. I don't know the impact of this change and I dont know if it is correct, but I think you should explain why this change of behavior is fine. Just my 2c. CJ > } > > static int
On Thu, Feb 13, 2025 at 02:39:25PM -0300, Guilherme Giacomo Simoes wrote: > The `get_power()` and `set_power()` function pointers in the > `cpci_hp_controller ops` struct were declared but never implemented by > any driver. To improve code readability and reduce resource usage, > remove this pointers and replace with a `flags` field. > > Use the new `flags` field in `enable_slot()`, `disable_slot()`, and > `cpci_get_power_s atus()` to track the slot's power state using the > `SLOT_ENABLED` macro. > > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> > --- > drivers/pci/hotplug/cpci_hotplug.h | 3 +-- > drivers/pci/hotplug/cpci_hotplug_core.c | 21 +++++++-------------- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/hotplug/cpci_hotplug.h b/drivers/pci/hotplug/cpci_hotplug.h > index 03fa39ab0c88..c5cb12cad2b4 100644 > --- a/drivers/pci/hotplug/cpci_hotplug.h > +++ b/drivers/pci/hotplug/cpci_hotplug.h > @@ -44,8 +44,7 @@ struct cpci_hp_controller_ops { > int (*enable_irq)(void); > int (*disable_irq)(void); > int (*check_irq)(void *dev_id); > - u8 (*get_power)(struct slot *slot); > - int (*set_power)(struct slot *slot, int value); > + int flags; > }; > > struct cpci_hp_controller { > diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c > index d0559d2faf50..87a743c2a5f1 100644 > --- a/drivers/pci/hotplug/cpci_hotplug_core.c > +++ b/drivers/pci/hotplug/cpci_hotplug_core.c > @@ -27,6 +27,8 @@ > #define DRIVER_AUTHOR "Scott Murray <scottm@somanetworks.com>" > #define DRIVER_DESC "CompactPCI Hot Plug Core" > > +#define SLOT_ENABLED 0x00000001 > + > #define MY_NAME "cpci_hotplug" > > #define dbg(format, arg...) \ > @@ -71,13 +73,12 @@ static int > enable_slot(struct hotplug_slot *hotplug_slot) > { > struct slot *slot = to_slot(hotplug_slot); > - int retval = 0; > > dbg("%s - physical_slot = %s", __func__, slot_name(slot)); > > - if (controller->ops->set_power) > - retval = controller->ops->set_power(slot, 1); > - return retval; > + controller->ops->flags |= SLOT_ENABLED; There are no implementations of ->set_power() or ->get_power(), are there? If not, we can just remove them and the calls to them. I don't see why we should add SLOT_ENABLED. > + return 0; > } > > static int > @@ -109,11 +110,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) > } > cpci_led_on(slot); > > - if (controller->ops->set_power) { > - retval = controller->ops->set_power(slot, 0); > - if (retval) > - goto disable_error; > - } > + controller->ops->flags &= ~SLOT_ENABLED; > > slot->adapter_status = 0; > > @@ -129,11 +126,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) > static u8 > cpci_get_power_status(struct slot *slot) > { > - u8 power = 1; > - > - if (controller->ops->get_power) > - power = controller->ops->get_power(slot); > - return power; > + return controller->ops->flags & SLOT_ENABLED; > } > > static int > -- > 2.34.1 >
Bjorn Helgaas <helgaas@kernel.org> wrote: > There are no implementations of ->set_power() or ->get_power(), are > there? If not, we can just remove them and the calls to them. > > I don't see why we should add SLOT_ENABLED. Are not implementations of set_power() and get_power(). I removed this funcions and in enable_slot(), disable_slot() and cpci_get_power_status() I use a `flags` field that I create in cpci_hp_controller_ops struct. I created this `flags` for store a power_status and use this in enable_slot(), disable_slot() and cpci_get_power_status() that before uses a set_power() and get_power(). I do this way, because I seeing this patter in another pci subsystems. In adittion on this, the flags can be used for store anothers values. But the Christophe JAILLET <christophe.jaillet@wanadoo.fr> say: "If neither get_power nor set_power where defined in any driver, then cpci_get_power_status() was always returning 1. IIUC, now it may return 1 or 0 depending of if enable_slot() or disable_slot() have been called." Do you think that is better we only return 1 in pci_get_power_status() and remove SLOT_ENABLED and `flags` field? Thanks, Guilherme
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > If neither get_power nor set_power where defined in any driver, then > cpci_get_power_status() was always returning 1. > > IIUC, now it may return 1 or 0 depending of if enable_slot() or > disable_slot() have been called. You is right... ever return 1, but, this is a expected behavior? Don't seems for me, that ever return 1 is the right way. > I don't know the impact of this change and I dont know if it is correct, > but I think you should explain why this change of behavior is fine. I submitt this patch only with intention that save resources removing the get_power and set_power pointers and yours calls. Thoughts ?? Thanks, Guilherme
Le 15/02/2025 à 03:10, Guilherme Giacomo Simoes a écrit : > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: >> If neither get_power nor set_power where defined in any driver, then >> cpci_get_power_status() was always returning 1. >> >> IIUC, now it may return 1 or 0 depending of if enable_slot() or >> disable_slot() have been called. > You is right... ever return 1, but, this is a expected behavior? > Don't seems for me, that ever return 1 is the right way. > >> I don't know the impact of this change and I dont know if it is correct, >> but I think you should explain why this change of behavior is fine. > I submitt this patch only with intention that save resources removing the > get_power and set_power pointers and yours calls. So, if unsure if the change of behavior you introduce is correct, you should only do what you wanted to do. If you still want to propose the other change, you should do the 2 things in 2 different steps. CJ > > Thoughts ?? > > Thanks, > Guilherme > >
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > So, if unsure if the change of behavior you introduce is correct, you > should only do what you wanted to do. > > If you still want to propose the other change, you should do the 2 > things in 2 different steps. Okay, I will send a patch with only (g|s)et_power() removed. Thanks, Guilherme
diff --git a/drivers/pci/hotplug/cpci_hotplug.h b/drivers/pci/hotplug/cpci_hotplug.h index 03fa39ab0c88..c5cb12cad2b4 100644 --- a/drivers/pci/hotplug/cpci_hotplug.h +++ b/drivers/pci/hotplug/cpci_hotplug.h @@ -44,8 +44,7 @@ struct cpci_hp_controller_ops { int (*enable_irq)(void); int (*disable_irq)(void); int (*check_irq)(void *dev_id); - u8 (*get_power)(struct slot *slot); - int (*set_power)(struct slot *slot, int value); + int flags; }; struct cpci_hp_controller { diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c index d0559d2faf50..87a743c2a5f1 100644 --- a/drivers/pci/hotplug/cpci_hotplug_core.c +++ b/drivers/pci/hotplug/cpci_hotplug_core.c @@ -27,6 +27,8 @@ #define DRIVER_AUTHOR "Scott Murray <scottm@somanetworks.com>" #define DRIVER_DESC "CompactPCI Hot Plug Core" +#define SLOT_ENABLED 0x00000001 + #define MY_NAME "cpci_hotplug" #define dbg(format, arg...) \ @@ -71,13 +73,12 @@ static int enable_slot(struct hotplug_slot *hotplug_slot) { struct slot *slot = to_slot(hotplug_slot); - int retval = 0; dbg("%s - physical_slot = %s", __func__, slot_name(slot)); - if (controller->ops->set_power) - retval = controller->ops->set_power(slot, 1); - return retval; + controller->ops->flags |= SLOT_ENABLED; + + return 0; } static int @@ -109,11 +110,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) } cpci_led_on(slot); - if (controller->ops->set_power) { - retval = controller->ops->set_power(slot, 0); - if (retval) - goto disable_error; - } + controller->ops->flags &= ~SLOT_ENABLED; slot->adapter_status = 0; @@ -129,11 +126,7 @@ disable_slot(struct hotplug_slot *hotplug_slot) static u8 cpci_get_power_status(struct slot *slot) { - u8 power = 1; - - if (controller->ops->get_power) - power = controller->ops->get_power(slot); - return power; + return controller->ops->flags & SLOT_ENABLED; } static int
The `get_power()` and `set_power()` function pointers in the `cpci_hp_controller ops` struct were declared but never implemented by any driver. To improve code readability and reduce resource usage, remove this pointers and replace with a `flags` field. Use the new `flags` field in `enable_slot()`, `disable_slot()`, and `cpci_get_power_s atus()` to track the slot's power state using the `SLOT_ENABLED` macro. Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> --- drivers/pci/hotplug/cpci_hotplug.h | 3 +-- drivers/pci/hotplug/cpci_hotplug_core.c | 21 +++++++-------------- 2 files changed, 8 insertions(+), 16 deletions(-)