Thank you for you comments.
On Fri, 13 Feb 2009 13:53:13 -0700
Alex Chiang <achiang@xxxxxx> wrote:
> 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
It is nice.
I will try to use your patches.
> If resource_alignment is a per-device attribute, doesn't it make
> sense to put the file in /sys/bus/pci/devices/resource_alignment ?
I don't think it is easy to add "/sys/bus/pci/devices/resource_alignment".
In case of "/sys/bus/pci/resource_alignment", we can use BUS_ATTR.
In case of "/sys/bus/pci/devices/SSSS:BB:DD.F/resource_alignment", we
can use DEVICE_ATTR.
But in case of "/sys/bus/pci/devices/resource_alignment", what can we
use?
> > 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?
I will write the help.
> And you really want to default y?
Yes. This does not cause the issue, I think.
> > +/**
> > + * pci_specified_resource_alignemnt - get resource alignment specified by user.
> > + * @dev: the PCI device to get
> > + *
> > + * RETURNS: Resrouce alignment if it is specified.
> ^^^^^^^^
> Resource
Thanks.
I will fix it.
> > + * 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.
CordingStyle in the kernel tree says "Long strings are as well broken
into shorter strings". I would not like to break this.
> > +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.
I will fix it.
> 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.
See above.
> > #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
I will fix it.
> > + * Later on, kernel will assign page-aligned memory resource back
> ^^^^^^^^^^^^^^^^
> Later on, the kernel
I will fix it.
> > +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?
No reason.
I will fix it.
> > + return;
> > + }
> > +
> > + dev_info(&dev->dev, "Disable device and release resources.\n");
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Disabling device and releasing resources
I will fix it.
> > + 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 will fix it.
>
> > + 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...
I will fix it.
> > + * 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
I will fix it.
Thanks,
--
Yuji Shimada
--
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
