On Thu, Mar 15, 2012 at 04:29:30PM +1300, Alexey Korolev wrote:
On 14/03/12 13:48, Kevin O'Connor wrote:
On Tue, Mar 13, 2012 at 05:45:19PM +1300, Alexey Korolev wrote:
Added pci_region_entry structure and list operations to pciinit.c List is filled with entries during pci_check_devices. List is used just for printing space allocation if we were using lists. Next step will resource allocation using mapping functions.
[...]
- struct {
u32 addr;u32 size;int is64;- } bars[PCI_NUM_REGIONS];
[...]
Yes I see what you mean here.
Thanks - I do find this patch much easier to understand. I do have some comments on the patch (see below).
The code is being changed - it's not new code being added and old code being deleted - the patches need to reflect that.
Because of structural changes it is not possible to completely avoid this scenario where new code is added and old deleted. In this patch series I tried my best to make migration as obvious and safe as possible. So the existing approach (with your suggestions) for pciinit.c redesign is this:
- Introduce list operations
- Introduce pci_region_entry structure and add code which fills this new structure.
We don't modify resource addresses calculations, but we use pci_region_entry data to do resource assignment. 3. Modify resource addresses calculations to be based on linked lists of region entries. 4. Remove code which fills the arrays, remove use of arrays for mapping. (note 3&4 could be combined altogether but it will be harder to read then)
On patch 3/4, neither patch should introduce code that isn't actually used nor leave code that isn't used still in. So, for example, if the arrays are still used after patch 3 then it's okay to leave them to patch 4, but if they are no longer used at all they should be removed within patch 3.
Could you please have a look at the other parts in this series and let me know if you are happy about this approach, so I won't have to redo patchwork too many times?
patch 1/6 - I'd prefer to have a list.h with struct hlist_head/hlist_node and container_of before extending to other parts of seabios. That said, I'm okay with what you have for pciinit - we can introduce list.h afterwards.
patch 3/4 - was confusing to me as it added new code in one patch and removed the replaced code in the second patch.
patch 5 - looked okay to me.
Thanks for looking at this - I know it's time consuming. Given the churn in this area I want to make sure there is a good understanding before any big changes.
comments on the patch: [...]
+struct pci_region_entry * +pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
u32 size, int type, int is64bit)+{
- struct pci_region_entry *entry= malloc_tmp(sizeof(*entry));
- if (!entry) {
warn_noalloc();return NULL;
Minor - whitespace.
[...]
+static int pci_bios_check_devices(struct pci_bus *busses) { dprintf(1, "PCI: check devices\n");
struct pci_region_entry *entry;
// Calculate resources needed for regular (non-bus) devices. struct pci_device *pci;
@@ -378,19 +419,27 @@ static void pci_bios_check_devices(struct pci_bus *busses) struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)]; int i; for (i = 0; i < PCI_NUM_REGIONS; i++) {
u32 val, size;
u32 val, size, min_size;int type, is64bit;
Minor - I prefer to use C99 inline variable declarations though it isn't a requirement.
min_size = (type == PCI_REGION_TYPE_IO) ? (1<<PCI_IO_INDEX_SHIFT):(1<<PCI_MEM_INDEX_SHIFT);size = (size > min_size) ? size : min_size;
My only real comment: Why the min_size change? Is that a fix of some sort or is it related to the move to lists?
[...]
/****************************************************************
- Main setup code
Minor - whitespace change.
-Kevin