[Skiboot] [PATCH v9 11/22] core/pci: Return value for struct phb_ops::device_init

[Daniel Axtens]

Gavin Shan <gwshan at linux.vnet.ibm.com> writes:

> This adds "int" return value for struct phb_ops::device_init()
> so that the function can be called in pci_walk_dev() directly
> to reinitialize the PCI devices behind the specified slot in
> the following patches.
>
> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
> ---
>  core/pci.c     |  2 +-
>  hw/p7ioc-phb.c | 11 ++++++++---
>  hw/phb3.c      | 11 ++++++++---
>  include/pci.h  |  2 +-
>  4 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/core/pci.c b/core/pci.c
> index 1fac425..54509c6 100644
> --- a/core/pci.c
> +++ b/core/pci.c
> @@ -254,7 +254,7 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren
>  	 * Call PHB hook
>  	 */
>  	if (phb->ops->device_init)
> -		phb->ops->device_init(phb, pd);
> +		phb->ops->device_init(phb, pd, NULL);
>  
>  	return pd;
>   fail:
> diff --git a/hw/p7ioc-phb.c b/hw/p7ioc-phb.c
> index b743bba..0632242 100644
> --- a/hw/p7ioc-phb.c
> +++ b/hw/p7ioc-phb.c
> @@ -2198,7 +2198,9 @@ static void p7ioc_endpoint_init(struct phb *phb,
>  	pci_cfg_write32(phb, bdfn, aercap + PCIECAP_AER_CAPCTL, val32);
>  }
>  
> -static void p7ioc_device_init(struct phb *phb, struct pci_device *dev)
> +static int p7ioc_device_init(struct phb *phb,
> +			     struct pci_device *dev,
> +			     void *data __unused)
You change a few signatures here to include an __unused variable.
I wonder if it would be nicer to create some shims that take the unused
variable and not add the unused variable to these functions?

I also notice that in a few cases the functions are returning 0
unconditionally. I'm not sure if you change that in a later patch. If
you don't change it later, I think that's another good reason to move
that into a shim.

However, I'm not particularly fussy either way, and I realise there are
some downsides to having shims, like having to create an extra function
and function pointer, so if you or Stewart have strong feelings about it
I won't insist on it.


>  {
>  	int ecap = 0;
>  	int aercap = 0;
> @@ -2227,6 +2229,8 @@ static void p7ioc_device_init(struct phb *phb, struct pci_device *dev)
>  		p7ioc_switch_port_init(phb, dev, ecap, aercap);
>  	else
>  		p7ioc_endpoint_init(phb, dev, ecap, aercap);
> +
> +	return 0;
>  }
>  
>  static int64_t p7ioc_pci_reinit(struct phb *phb,
> @@ -2234,6 +2238,7 @@ static int64_t p7ioc_pci_reinit(struct phb *phb,
>  {
>  	struct pci_device *pd;
>  	uint16_t bdfn = data;
> +	int ret;
>  
>  	if (scope != OPAL_REINIT_PCI_DEV)
>  		return OPAL_PARAMETER;
> @@ -2242,8 +2247,8 @@ static int64_t p7ioc_pci_reinit(struct phb *phb,
>  	if (!pd)
>  		return OPAL_PARAMETER;
>  
> -	p7ioc_device_init(phb, pd);
> -	return OPAL_SUCCESS;
> +	ret = p7ioc_device_init(phb, pd, NULL);
> +	return ret ? OPAL_HARDWARE : OPAL_SUCCESS;
I understand that this is equivalent to:
  if (ret)
    return OPAL_HARDWARE
  return OPAL_SUCCESS

However, I think the ternary operator makes it a bit unclear.
I think (ret == 0) ? OPAL_SUCCESS : OPAL_HARDWARE
would be clearer, but this is an aesthetic opinion, and shouldn't stop
the code going in.

>  }
>  
>  static uint8_t p7ioc_choose_bus(struct phb *phb __unused,
> diff --git a/hw/phb3.c b/hw/phb3.c
> index 2053277..26c79dc 100644
> --- a/hw/phb3.c
> +++ b/hw/phb3.c
> @@ -536,7 +536,9 @@ static void phb3_check_device_quirks(struct phb *phb, struct pci_device *dev)
>  	}
>  }
>  
> -static void phb3_device_init(struct phb *phb, struct pci_device *dev)
> +static int phb3_device_init(struct phb *phb,
> +			    struct pci_device *dev,
> +			    void *data __unused)
>  {
>  	int ecap = 0;
>  	int aercap = 0;
> @@ -568,12 +570,15 @@ static void phb3_device_init(struct phb *phb, struct pci_device *dev)
>  		phb3_switch_port_init(phb, dev, ecap, aercap);
>  	else
>  		phb3_endpoint_init(phb, dev, ecap, aercap);
> +
> +	return 0;
>  }
>  
>  static int64_t phb3_pci_reinit(struct phb *phb, uint64_t scope, uint64_t data)
>  {
>  	struct pci_device *pd;
>  	uint16_t bdfn = data;
> +	int ret;
>  
>  	if (scope != OPAL_REINIT_PCI_DEV)
>  		return OPAL_PARAMETER;
> @@ -582,8 +587,8 @@ static int64_t phb3_pci_reinit(struct phb *phb, uint64_t scope, uint64_t data)
>  	if (!pd)
>  		return OPAL_PARAMETER;
>  
> -	phb3_device_init(phb, pd);
> -	return OPAL_SUCCESS;
> +	ret = phb3_device_init(phb, pd, NULL);
> +	return ret ? OPAL_HARDWARE : OPAL_SUCCESS;
>  }
>  
>  static int64_t phb3_presence_detect(struct phb *phb)
> diff --git a/include/pci.h b/include/pci.h
> index 1f648bd..0f56ed0 100644
> --- a/include/pci.h
> +++ b/include/pci.h
> @@ -264,7 +264,7 @@ struct phb_ops {
>  	 * and before probing further. It can alter things like scan_map
>  	 * for bridge ports etc...
>  	 */
> -	void (*device_init)(struct phb *phb, struct pci_device *device);
> +	int (*device_init)(struct phb *phb, struct pci_device *pd, void *data);
>  
>  	/*
>  	 * Device node fixup is called when the PCI device node is being
> -- 
> 2.1.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 859 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20151201/be53e26e/attachment.sig>