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

Mon May 2 07:58:15 UTC 2016

On 05/02/2016 10:39 AM, Ladi Prosek wrote:
> 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?

Because if you document for one caller, you should document it for every caller
of the function that actually makes the change. What about caller's caller?
What if in this function would call another function that also make some modification?
Will the comment of the caller would be the sum of all comments ?
It is very hard to maintain this.

This is why I would comment only on the original function.
But it is only a suggestion, nothing more :)

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

This register may be implemented as read-only for devices that burst two
or fewer data phases, but the hardwired value must be limited to 16 or less. (PCI spec)

Since the virtio devices are virtual, this part is not interesting.

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

Sure, you can rename or actually check the device is "enabled (has at least one of the IO/MEM)"
by the function or disabled (has none), otherwise return with error.
Is up to you.

Thanks,
Marcel

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