linux-kernel - Re: [PATCH] PCI: ibmphp: Adjusting the parameter list of a function

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: <20220818230755.GA2403414@bhelgaas>
Date:   Thu, 18 Aug 2022 18:07:55 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Abd-Alrhman Masalkhi <abd.masalkhi@...il.com>
Cc:     bhelgaas@...gle.com, kw@...ux.com, os.vaslot@...il.com,
        makvihas@...il.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: ibmphp: Adjusting the parameter list of a function

On Sun, Aug 14, 2022 at 05:19:56PM +0200, Abd-Alrhman Masalkhi wrote:
> ibmphp_init_devno() takes a struct slot **, and it should be changed
> to take struct slot * instead.

The commit log needs to explain *why* it should be changed from
"struct slot **" to "struct slot *".

And the subject line should give a clue about that, too.

Both subject and commit log should be "imperative," e.g., they should
read as if you were giving a command.  "PCI: ibmphp: Convert ..."

This commit log says what *should* happen.  It should read as though
it's a command carried out by the commit: "Convert 'struct slot **' to
'struct slot *' because ..."

> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@...il.com>
> ---
>  drivers/pci/hotplug/TODO          |  3 ---
>  drivers/pci/hotplug/ibmphp.h      |  3 ++-
>  drivers/pci/hotplug/ibmphp_core.c | 30 +++++++++++++++---------------
>  drivers/pci/hotplug/ibmphp_ebda.c |  2 +-
>  4 files changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO
> index 88f217c82b4f..63a9fed407ed 100644
> --- a/drivers/pci/hotplug/TODO
> +++ b/drivers/pci/hotplug/TODO
> @@ -30,9 +30,6 @@ ibmphp:
>    or ibmphp should store a pointer to its bus in struct slot.  Probably the
>    former.
>  
> -* ibmphp_init_devno() takes a struct slot **, it could instead take a
> -  struct slot *.
> -
>  * The return value of pci_hp_register() is not checked.
>  
>  * The various slot data structures are difficult to follow and need to be
> diff --git a/drivers/pci/hotplug/ibmphp.h b/drivers/pci/hotplug/ibmphp.h
> index 0399c60d2ec1..148a421eb262 100644
> --- a/drivers/pci/hotplug/ibmphp.h
> +++ b/drivers/pci/hotplug/ibmphp.h
> @@ -734,7 +734,8 @@ struct controller {
>  
>  /* Functions */
>  
> -int ibmphp_init_devno(struct slot **);	/* This function is called from EBDA, so we need it not be static */
> +/* ibmphp_init_devno() is called from EBDA, so we need it not be static */
> +int ibmphp_init_devno(struct slot *cur_slot);
>  int ibmphp_do_disable_slot(struct slot *slot_cur);
>  int ibmphp_update_slot_info(struct slot *);	/* This function is called from HPC, so we need it to not be static */
>  int ibmphp_configure_card(struct pci_func *, u8);
> diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
> index 197997e264a2..107752b11f2c 100644
> --- a/drivers/pci/hotplug/ibmphp_core.c
> +++ b/drivers/pci/hotplug/ibmphp_core.c
> @@ -109,7 +109,7 @@ static int __init get_max_slots(void)
>   * Parameters: struct slot
>   * Returns 0 or errors
>   */
> -int ibmphp_init_devno(struct slot **cur_slot)
> +int ibmphp_init_devno(struct slot *cur_slot)
>  {
>  	struct irq_routing_table *rtable;
>  	int len;
> @@ -130,21 +130,21 @@ int ibmphp_init_devno(struct slot **cur_slot)
>  		return -1;
>  	}
>  	for (loop = 0; loop < len; loop++) {
> -		if ((*cur_slot)->number == rtable->slots[loop].slot &&
> -		    (*cur_slot)->bus == rtable->slots[loop].bus) {
> -			(*cur_slot)->device = PCI_SLOT(rtable->slots[loop].devfn);
> +		if (cur_slot->number == rtable->slots[loop].slot &&
> +		    cur_slot->bus == rtable->slots[loop].bus) {
> +			cur_slot->device = PCI_SLOT(rtable->slots[loop].devfn);
>  			for (i = 0; i < 4; i++)
> -				(*cur_slot)->irq[i] = IO_APIC_get_PCI_irq_vector((int) (*cur_slot)->bus,
> -						(int) (*cur_slot)->device, i);
> -
> -			debug("(*cur_slot)->irq[0] = %x\n",
> -					(*cur_slot)->irq[0]);
> -			debug("(*cur_slot)->irq[1] = %x\n",
> -					(*cur_slot)->irq[1]);
> -			debug("(*cur_slot)->irq[2] = %x\n",
> -					(*cur_slot)->irq[2]);
> -			debug("(*cur_slot)->irq[3] = %x\n",
> -					(*cur_slot)->irq[3]);
> +				cur_slot->irq[i] = IO_APIC_get_PCI_irq_vector((int) cur_slot->bus,
> +						(int) cur_slot->device, i);
> +
> +			debug("cur_slot->irq[0] = %x\n",
> +					cur_slot->irq[0]);
> +			debug("cur_slot->irq[1] = %x\n",
> +					cur_slot->irq[1]);
> +			debug("cur_slot->irq[2] = %x\n",
> +					cur_slot->irq[2]);
> +			debug("cur_slot->irq[3] = %x\n",
> +					cur_slot->irq[3]);
>  
>  			debug("rtable->exclusive_irqs = %x\n",
>  					rtable->exclusive_irqs);
> diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
> index 7fb75401ad8a..5c7821ad9c2c 100644
> --- a/drivers/pci/hotplug/ibmphp_ebda.c
> +++ b/drivers/pci/hotplug/ibmphp_ebda.c
> @@ -880,7 +880,7 @@ static int __init ebda_rsrc_controller(void)
>  			if (rc)
>  				goto error;
>  
> -			rc = ibmphp_init_devno(&tmp_slot);
> +			rc = ibmphp_init_devno(tmp_slot);
>  			if (rc)
>  				goto error;
>  			tmp_slot->hotplug_slot.ops = &ibmphp_hotplug_slot_ops;
> -- 
> 2.29.0.rc1.dirty
>