linux-kernel - RE: [PATCH 1/4 v2] PCI: introduce new base functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 10 Sep 2008 16:17:18 +0800
From:	"Zhao, Yu" <yu.zhao@...el.com>
To:	"Alex Chiang" <achiang@...com>
Cc:	"Jesse Barnes" <jbarnes@...tuousgeek.org>,
	<linux-pci@...r.kernel.org>,
	"Randy Dunlap" <randy.dunlap@...cle.com>,
	"Greg KH" <greg@...ah.com>,
	"Grant Grundler" <grundler@...isc-linux.org>,
	"Matthew Wilcox" <matthew@....cx>, <linux-kernel@...r.kernel.org>,
	<kvm@...r.kernel.org>, <virtualization@...ts.linux-foundation.org>,
	<xen-devel@...ts.xensource.com>
Subject: RE: [PATCH 1/4 v2] PCI: introduce new base functions

On Tuesday, September 02, 2008 12:16 AM, Alex Chiang wrote:
>* Zhao, Yu <yu.zhao@...el.com>:
>> Some basic changes to allocation bus range, MMIO resource for SR-IOV device.
>
>This following comment is a bit confusing:
>> And add new sysfs entry to hotplug core to pass parameter to a
>> slot, which will be used by SR-IOV code.
>
>I was reading this patch, expecting to see a change to the
>hotplug core _API_ taking a param, not just a new sysfs entry.
>
>I would suggest rewording this part of the changelog as:
>
>	Add new sysfs file 'param' to /sys/bus/pci/slots/.../
>	which allows the user to pass a parameter to a slot. This
>	parameter will be used by the SR-IOV code.
>
>More about this new 'param' file below.
>
>>
>
>Please break the 80-column "rule" and make this all one line, to
>help with greppability.
>
>I know the prior version had it broken across two lines too, but
>we can improve it now. :)

Sure, will do this in next version :-)

>
>> +			continue;
>> +		}
>> +		child->is_added = 1;
>> +		retval = device_create_file(&child->dev,
>> +					    &dev_attr_cpuaffinity);
>> +		if (retval)
>> +			dev_err(&dev->dev, "Error creating cpuaffinity"
>> +					   " file, continuing...\n");
>
>This one too, please.
>
>> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
>> index e1098c3..f6f2a59 100644
>> --- a/drivers/pci/proc.c
>> +++ b/drivers/pci/proc.c
>> @@ -352,15 +352,16 @@ static int show_device(struct seq_file *m, void *v)
>>  			dev->vendor,
>>  			dev->device,
>>  			dev->irq);
>> -	/* Here should be 7 and not PCI_NUM_RESOURCES as we need to preserve
>compatibility */
>> -	for (i=0; i<7; i++) {
>> +
>> +	/* only print standard and ROM resources to preserve compatibility */
>> +	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>
>Why not:
>
>	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
>
>Looks like the tradeoff is explicit mention of PCI_ROM_RESOURCE
>vs using a non-standard C idiom (personally, I'm not a huge fan
>of <= in a for loop, but ymmv).
>
>Again, this is a minor nit, feel free to ignore.

It can be PCI_BRIDGE_RESOURCES, because there may be some non-standard resources following PCI_ROM_RESOURCE and before PCI_BRIDGE_RESOURCES.

For example, a standard PCI device has following resources:
	0 - 5   BARs
	6       ROM
	7 - 10  Bridge

After SR-IOV is enabled, it becomes
	0 - 5   standard BARs
	6       Rom
     7 - 12  SR-IOV BARs
	13 - 16 Bridge

>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index c0e1400..687be00 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -76,7 +76,29 @@ enum pci_mmap_state {
>>  #define PCI_DMA_FROMDEVICE	2
>>  #define PCI_DMA_NONE		3
>>
>> -#define DEVICE_COUNT_RESOURCE	12
>> +/*
>> + *  For PCI devices, the region numbers are assigned this way:
>> + */
>> +enum {
>> +	/* 0-5	standard PCI regions */
>> +	PCI_STD_RESOURCE,
>> +
>> +	/* expansion ROM */
>> +	PCI_ROM_RESOURCE = 6,
>> +
>> +	/* address space assigned to buses behind the bridge */
>> +#ifndef PCI_BRIDGE_NUM_RES
>> +#define PCI_BRIDGE_NUM_RES 4
>> +#endif
>> +	PCI_BRIDGE_RESOURCES,
>> +	PCI_BRIDGE_RES_END = PCI_BRIDGE_RESOURCES + PCI_BRIDGE_NUM_RES - 1,
>> +
>> +	/* total resources associated with a PCI device */
>> +	PCI_NUM_RESOURCES,
>> +
>> +	/* preserve this for compatibility */
>> +	DEVICE_COUNT_RESOURCE
>> +};
>
>Ouch, this enum makes my head hurt a little. Care to put in a
>comment for PCI_BRIDGE_RES_END, saying that it has a value of 10?

Same as above, the PCI_BRIDGE_RES_END varies when some features is enabled or disabled.

Thank you very much for carefully reviewing these patches. I'd like to invite you to review next version again if it's convenient for you.

>
>Thanks,
>
>/ac

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/