[ipxe-devel] [PATCH 1/2] [pci] Add enable_pci_device and disable_pci_device
Ladi Prosek
lprosek at redhat.com
Mon May 2 08:12:32 UTC 2016
On Mon, May 2, 2016 at 9:58 AM, Marcel Apfelbaum <marcel at redhat.com> wrote:
> 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.
Oh no, the slippery slope argument :) Obviously, the comments should
somewhat reflect the level of abstraction at which the function
operates. In this case it's just a wrapper, which is not abstracting
anything away, so I believe that the benefit of keeping the original
comment (the functionality hasn't changed!) outweighs the potential
maintenance problems.
> This is why I would comment only on the original function.
> But it is only a suggestion, nothing more :)
And I very much appreciate it, and all the other comments and
suggestions as well.
Thank you!
Ladi
>>
>> 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 );
>>>>
>>>
>
More information about the ipxe-devel
mailing list