Change in coreboot[master]: src/soc/intel/common/itss: Add support to get interrupt table for PCI...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 )
Change subject: src/soc/intel/common/itss: Add support to get interrupt table for PCI devices ......................................................................
Patch Set 8:
(8 comments)
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/irq.c:
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 32: create_device_node nit: This doesn't have anything to do with a device, rather with a list of IRQ information; maybe create_irq_node?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 36: node = (struct irq_node *)malloc(sizeof(struct irq_node)); : node->next = NULL; What if malloc fails?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 41: dev nit: This isn't a device, so maybe 'node' would be a better name?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 71: static struct irq_node *info; : static int slot; : /* PIRx mapped from IRQ# 16:23 starting from PIRQA */ : static int int_line = PIRQA_APIC_IRQ; : static int int_lpss = PIRQA_APIC_IRQ; Will this really be called frequently enough to warrant storing all this state between calls?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 93: * only interrupt 16-23 can be shared */ : if (int_line > 23) If the comment is true, should you check if " || int_line < 16" as well ?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 102: static bool is_irq_dev(struct device *dev) : { : static size_t size; : static const int *devlist; : : if (!devlist) : devlist = soc_irq_devices_list(&size); : for (int i = 0; i < size; i++) { : if (dev->path.pci.devfn == devlist[i]) : return true; : } : return false; : } : Is this really going to be called frequently enough to need to store the devlist between calls?
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 141: void *int_cfg_ptr Can this be of type "struct dev_irq *" instead? void* makes me nervous :)
https://review.coreboot.org/c/coreboot/+/34089/8/src/soc/intel/common/block/... PS8, Line 148: if (!irq_config) : return -1; To be safe, and potentially not need callers to check for NULL, could this set *int_num = 0 as well?
-
Tim Wawrzynczak (Code Review)