[ipxe-devel] [PATCH 1/2] [pci] Add enable_pci_device and disable_pci_device

[Marcel Apfelbaum]

Hi Ladi,

On 04/28/2016 06:34 PM, Ladi Prosek wrote:
> adjust_pci_device unconditionally enables both memory and I/O space
> access, which may not be necessary.

The above problem is not addressed in this patch,
people my get confused.

  There was also no way to revert
> the device back to the original state.

>
> This commit adds two new functions. enable_pci_device gives drivers
> finer control over how the PCI device is initialized and
> disable_pci_device is the shutdown path counterpart.
>
> Signed-off-by: Ladi Prosek <lprosek at redhat.com>
> Suggested-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>   src/drivers/bus/pci.c  | 41 +++++++++++++++++++++++++++++++++++------
>   src/include/ipxe/pci.h |  2 ++
>   2 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/src/drivers/bus/pci.c b/src/drivers/bus/pci.c
> index 6fbedd9..bdff2b9 100644
> --- a/src/drivers/bus/pci.c
> +++ b/src/drivers/bus/pci.c
> @@ -143,16 +143,32 @@ static void pci_read_bases ( struct pci_device *pci ) {
>    *
>    * @v pci		PCI device
>    *
> - * Set device to be a busmaster in case BIOS neglected to do so.  Also
> - * adjust PCI latency timer to a reasonable value, 32.
> + * Set device to be a busmaster in case BIOS neglected to do so.  Enable

You can use the opportunity for changing busmaster to 'bus master' or 'Bus Master'

> + * both memory and I/O space access.  Also adjust PCI latency timer to a
> + * reasonable value, 32.

Adjusting the latency to 32 is done by an inner function, you may want to remove
this comment here.

>    */
>   void adjust_pci_device ( struct pci_device *pci ) {
> +	enable_pci_device ( pci,
> +			    PCI_COMMAND_MASTER | PCI_COMMAND_MEM |
> +			    PCI_COMMAND_IO );
> +}
> +
> +/**
> + * Enable PCI device
> + *
> + * @v pci		PCI device
> + * @v flags		PCI command flags to enable
> + * @ret old_flags	Original PCI command flags
> + *
> + * Enable device in case BIOS neglected to do so.  Also adjust PCI latency
> + * timer to a reasonable value, 32.
> + */
> +unsigned enable_pci_device ( struct pci_device *pci, unsigned flags ) {
>   	unsigned short new_command, pci_command;
>   	unsigned char pci_latency;
>
>   	pci_read_config_word ( pci, PCI_COMMAND, &pci_command );
> -	new_command = ( pci_command | PCI_COMMAND_MASTER |
> -			PCI_COMMAND_MEM | PCI_COMMAND_IO );
> +	new_command = ( pci_command | flags );
>   	if ( pci_command != new_command ) {
>   		DBGC ( pci, PCI_FMT " device not enabled by BIOS! Updating "
>   		       "PCI command %04x->%04x\n",

The debug message is not true anymore. Maybe the BIOS enabled IO and now we
want to enable also MEM.

> @@ -160,12 +176,25 @@ void adjust_pci_device ( struct pci_device *pci ) {
>   		pci_write_config_word ( pci, PCI_COMMAND, new_command );
>   	}
>
> -	pci_read_config_byte ( pci, PCI_LATENCY_TIMER, &pci_latency);
> +	pci_read_config_byte ( pci, PCI_LATENCY_TIMER, &pci_latency );
>   	if ( pci_latency < 32 ) {
>   		DBGC ( pci, PCI_FMT " latency timer is unreasonably low at "
>   		       "%d. Setting to 32.\n", PCI_ARGS ( pci ), pci_latency );
> -		pci_write_config_byte ( pci, PCI_LATENCY_TIMER, 32);
> +		pci_write_config_byte ( pci, PCI_LATENCY_TIMER, 32 );
>   	}
> +	return pci_command;
> +}
> +
> +/**
> + * Disable PCI device
> + *
> + * @v pci		PCI device
> + * @v old_flags		Flags to restore, returned from enable_pci_device
> + *
> + * Restore device to state it had before we enabled it.
> + */
> +void disable_pci_device ( struct pci_device *pci, unsigned old_flags ) {
> +	pci_write_config_word ( pci, PCI_COMMAND, old_flags );
>   }

I have a slight problem with the name of the function which does no really
reflects what the function does. If we look at the debug message example,
it is possible the device remains enabled, but only using IO.

A similar but smaller issue has 'enable_pci_device. You don't check
that at least an "enabling" bit is passed.

I would suggest renaming this function to something like 'pci_restore_cmd'
or simply call pci_write_config_word directly.

>
>   /**
> diff --git a/src/include/ipxe/pci.h b/src/include/ipxe/pci.h
> index 0c6bc7e..3c613c2 100644
> --- a/src/include/ipxe/pci.h
> +++ b/src/include/ipxe/pci.h
> @@ -278,6 +278,8 @@ struct pci_driver {
>   	PCI_FUNC ( (pci)->busdevfn )
>
>   extern void adjust_pci_device ( struct pci_device *pci );
> +extern unsigned enable_pci_device ( struct pci_device *pci, unsigned flags );
> +extern void disable_pci_device ( struct pci_device *pci, unsigned flags );
>   extern unsigned long pci_bar_start ( struct pci_device *pci,
>   				     unsigned int reg );
>   extern int pci_read_config ( struct pci_device *pci );
>