Hi Yuji-san,
Jesse asked me to review this from the user interface
perspective.
> If you want to assign the resource at run-time, please set
> "/sys/bus/pci/resource_alignment" file, and hot-remove the device and
> hot-add the device. For this purpose, fakephp can be used.
We can use fakephp for now.
Later, when my PCI bus rescan work is complete, we can use that
instead.
> The format of the file is the same with boot parameter. You can use
> "," instead of ";".
>
> This is example:
>
> # /sbin/modprobe fakephp
> # cd /sys/bus/pci
> # echo -n 20@0d:00.0 > resource_alignment
> # cat slots/fake13/address
> 0000:0d:00
> # echo -n 0 > slots/fake13/power
> # echo -n 1 > slots/fake1/power
For example:
# cd /sys/bus/pci
# echo -n 20@0d:00.0 > resource_alignment
# echo 1 > devices/.../remove
# echo 1 > rescan
If resource_alignment is a per-device attribute, doesn't it make
sense to put the file in /sys/bus/pci/devices/resource_alignment ?
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 2a4501d..cc2c777 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -59,3 +59,8 @@ config HT_IRQ
> This allows native hypertransport devices to use interrupts.
>
> If unsure say Y.
> +
> +config RESOURCE_ALIGNMENT_PARAM
> + bool
> + depends on PCI_QUIRKS
> + default y
No Kconfig help?
And you really want to default y?
> +/**
> + * pci_specified_resource_alignemnt - get resource alignment specified by user.
> + * @dev: the PCI device to get
> + *
> + * RETURNS: Resrouce alignment if it is specified.
^^^^^^^^
Resource
> + * Zero if it is not specified.
> + */
> +resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
> +{
> + int seg, bus, slot, func, align_order, count;
> + resource_size_t align = 0;
> + char *p;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&resource_alignment_lock, flags);
> + p = resource_alignment_param;
> + while (*p) {
> + count = 0;
> + if (sscanf(p, "%d%n", &align_order, &count) == 1 &&
> + p[count] == '@') {
> + p += count + 1;
> + } else {
> + align_order = -1;
> + }
> + if (sscanf(p, "%x:%x:%x.%x%n",
> + &seg, &bus, &slot, &func, &count) != 4) {
> + seg = 0;
> + if (sscanf(p, "%x:%x.%x%n",
> + &bus, &slot, &func, &count) != 3) {
> + /* Invalid format */
> + printk(KERN_ERR "PCI: Can't parse "
> + "resource_alignment parameter: %s\n",
I personally prefer all printk text to be on one line to improve
greppability. I think it's one of the valid reasons to break
80-column rule.
> +ssize_t pci_set_resource_alignment_param(const char *buf, size_t count)
> +{
> + unsigned long flags;
> + if (RESOURCE_ALIGNMENT_PARAM_SIZE - 1 < count)
> + count = RESOURCE_ALIGNMENT_PARAM_SIZE - 1;
Can you reverse the logic here so it reads a little more
naturally?
if (count > RESOURCE_ALIGNMENT_PARAM_SIZE - 1)
count = RESOURCE_ALIGNMENT_PARAM_SIZE - 1;
Now it is clear you are checking for a value of count that is too
large.
Also, why do you need the magic "- 1"? (Sorry, I was reading your
patch very quickly, so didn't remember any comments you may have
had about RESOURCE_ALIGNMENT_PARAM_SIZE, but then again, it is
likely that anyone in the future will have the same question.)
> +BUS_ATTR(resource_alignment, 0644, pci_resource_alignment_show,
> + pci_resource_alignment_store);
> +
> +static int __init pci_resource_alignment_sysfs_init(void)
> +{
> + return bus_create_file(&pci_bus_type,
> + &bus_attr_resource_alignment);
> +}
Again, I wonder why this is a BUS_ATTR rather than a per-device
attribute.
Obviously, you can't put it in /sys/bus/pci/devices/.../ because
the device will go away during a hotplug, but I think that
putting it in /sys/bus/pci/devices/ would be a bit better.
> #ifdef CONFIG_PCI_QUIRKS
> +/*
> + * This quirk function disables the device and releases resources
> + * which is specified by kernel's boot parameter 'pci=resource_alignment='.
> + * It also round up size to specified alignment.
^^^^^
rounds
> + * Later on, kernel will assign page-aligned memory resource back
^^^^^^^^^^^^^^^^
Later on, the kernel
> +static void __devinit quirk_resource_alignment(struct pci_dev *dev)
> +{
> + int i;
> + struct resource *r;
> + resource_size_t align, size;
> +
> + if (!pci_is_reassigndev(dev))
> + return;
> +
> + if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
> + (dev->class >> 8) == PCI_CLASS_BRIDGE_HOST) {
> + /* PCI Host Bridge isn't a target device */
> + dev_warn(&dev->dev,
> + "Can't reassign resources to Host Bridge.\n");
^^^^^^^^^^^^
why is host
bridge capitalized?
> + return;
> + }
> +
> + dev_info(&dev->dev, "Disable device and release resources.\n");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Disabling device and releasing resources
> + pci_disable_device(dev);
> +
> + align = pci_specified_resource_alignment(dev);
> + for (i=0; i < PCI_NUM_RESOURCES; i++) {
> + r = &dev->resource[i];
> + if (!(r->flags & IORESOURCE_MEM))
> + continue;
> + size = r->end - r->start + 1;
> + if (size < align) {
> + size = align;
> + dev_info(&dev->dev,
> + "Round up size of resource #%d to %#llx.\n",
^^^^^
Rounding
> + i, (unsigned long long)size);
> + }
> + r->end = size - 1;
> + r->start = 0;
> +
> + if (i < PCI_BRIDGE_RESOURCES) {
> + pci_update_resource(dev, i);
> + }
> + }
> + /* need to disable bridge's resource window,
> + * to make kernel enable to reassign new resource
^^^^^^^^^^^^^^^^^^^^^
to enable the kernel to reassign...
> + * window later on.
> + */
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> + (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> + pci_disable_bridge_window(dev);
> + }
> +}
>
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, quirk_resource_alignment);
> +#ifdef CONFIG_PCI_QUIRKS
> +void pci_disable_bridge_window(struct pci_dev *dev)
> +{
> + dev_dbg(&dev->dev, "Disable bridge window.\n");
^^^^^^^
Disabling
Thanks.
/ac
--
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
