linux-kernel - Re: [RESEND PATCH] PCI: cpci: remove unused fields

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a1af3a07-1e76-488b-82f7-87b3a4907f26@wanadoo.fr>
Date: Thu, 13 Feb 2025 21:44:27 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Guilherme Giacomo Simoes <trintaeoitogc@...il.com>, scott@...teful.org,
 bhelgaas@...gle.com
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH] PCI: cpci: remove unused fields

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@...il.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@...anetworks.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