Re: [PATCH] pciutils: Display physical slot information in lspci -v

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Alex!

> We've been exposing slot information in /sys/bus/pci/slots for a
> long time now (as long as a hotplug driver or slot detection
> driver like pci_slot is loaded).
> 
> Let's make life easy for our users and display that information
> in lspci. If slot entries appear in /sys/bus/pci/slots/,
> correlate them to PCI devices, and display the information when
> lspci -v is issued.

I like the feature, but the current implementation does not make me very
happy.

First of all: scanning the slot directory again and again on each
device lookup is a waste of resources, especially when it is not clear
that the user of the library is interested in slots. Also, it introduces
an ABI incompatibility: a binary does not know whether the library
it is currently linked with has phy_slot in struct pci_dev or not.

I think that we should use pci_fill_info() for that. When somebody
calls pci_fill_info() with a PCI_FILL_PHYS_SLOT flag for the first time,
the sysfs backend scans the slot directory and fills in the slot name
for all known devices.

Could you please do it this way?

Also, please base the patch on the current development tree in the
GIT repository (or the 3.0-alpha2 release from the FTP).

> +	d->phy_slot = pci_malloc(a, strlen(entry->d_name) + 1);

This memory is leaked.

> +  if (p->phy_slot)
> +      printf("\tPhysical Slot: %s\n", p->phy_slot);

It would be nice to have the slot available in the machine-readable
output format, too. (Please document that in the man page.)

				Have a nice fortnight
-- 
Martin `MJ' Mares                          <mj@xxxxxx>   http://mj.ucw.cz/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
There are two rules to success: 1. Never tell all you know.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux