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

[Ladi Prosek]

On Sun, May 1, 2016 at 2:24 PM, Marcel Apfelbaum <marcel at redhat.com> wrote:
> 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.

I'll fix the commit message, thanks!

>  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'

Will do.

>> + * 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.

Hmm.. not sure about this one. It's still done by the function,
doesn't matter if directly or somewhere down the calltree, why would
you not want it documented in the comment?

By the way, I noticed that the latency timer writes don't stick. Is it
expected for virtual devices to have a read-only implementation of
this register?

>>    */
>>   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.

Will fix, thanks.

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

I'll rename them, thanks.

>
>>
>>   /**
>> 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 );
>>
>