Thank you for your review.
On Tue, 3 Mar 2009 16:43:49 +0800
Yu Zhao <yu.zhao@xxxxxxxxx> wrote:
> On Mon, Mar 02, 2009 at 03:08:15PM +0800, Yuji Shimada wrote:
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index b182626..290e701 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -1756,6 +1756,15 @@ and is between 256 and 4096 characters. It is defined in the file
> > cbmemsize=nn[KMG] The fixed amount of bus space which is
> > reserved for the CardBus bridge's memory
> > window. The default value is 64 megabytes.
> > + resource_alignment=
> > + Format:
> > + [<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
> > + Specifies alignment and device to reassign
> > + aligned memory resources.
> > + If <order of align> is not specified,
> > + PAGE_SIZE is used as alignment.
> > + PCI-PCI bridge can be specified, if resource
> > + windows need to be expanded.
> >
> > pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
> > Management.
>
> Please remove trailing spaces in the patch.
I will remove the trailing spaces.
> > +/**
> > + * pci_specified_resource_alignemnt - get resource alignment specified by user.
> > + * @dev: the PCI device to get
> > + *
> > + * RETURNS: Resource alignment if it is specified.
> > + * 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);
>
> These functions won't be called in interrupt handler, right? So it doesn't
> need to disable interrupt.
I will use spin_lock instead of spin_lock_irqsave.
> > + 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",
> > + p);
> > + break;
> > + }
> > + }
> > + p += count;
> > + if (seg == pci_domain_nr(dev->bus) &&
> > + bus == dev->bus->number &&
> > + slot == PCI_SLOT(dev->devfn) &&
> > + func == PCI_FUNC(dev->devfn)) {
> > + if (align_order == -1) {
> > + align = PAGE_SIZE;
> > + } else {
> > + align = 1 << align_order;
> > + }
> > + /* Found */
> > + break;
> > + }
> > + if (*p != ';' && *p != ',') {
> > + /* End of param or invalid format */
> > + break;
> > + }
> > + p++;
> > + }
> > + spin_unlock_irqrestore(&resource_alignment_lock, flags);
> > + return align;
> > +}
> > +
> > +/**
> > + * pci_is_reassigndev - check if specified PCI is target device to reassign
> > + * @dev: the PCI device to check
> > + *
> > + * RETURNS: non-zero for PCI device is a target device to reassign,
> > + * or zero is not.
> > + */
> > +int pci_is_reassigndev(struct pci_dev *dev)
> > +{
> > + return (pci_specified_resource_alignment(dev) != 0);
> > +}
>
> Looks like this wrapper is unnecessary.
I think it is better to keep this wrapper. quirk_resource_alignment
becomes intuitive.
> > +
> > +ssize_t pci_set_resource_alignment_param(const char *buf, size_t count)
> > +{
> > + unsigned long flags;
> > + if (count > RESOURCE_ALIGNMENT_PARAM_SIZE - 1)
> > + count = RESOURCE_ALIGNMENT_PARAM_SIZE - 1;
> > + spin_lock_irqsave(&resource_alignment_lock, flags);
> > + strncpy(resource_alignment_param, buf, count);
> > + resource_alignment_param[count] = '\0';
> > + spin_unlock_irqrestore(&resource_alignment_lock, flags);
> > + return count;
> > +}
> > +
> > +ssize_t pci_get_resource_alignment_param(char *buf, size_t size)
> > +{
> > + unsigned long flags;
> > + size_t count;
> > + spin_lock_irqsave(&resource_alignment_lock, flags);
> > + count = snprintf(buf, size, "%s", resource_alignment_param);
> > + spin_unlock_irqrestore(&resource_alignment_lock, flags);
> > + return count;
> > +}
> > +
> > +static void __devinit pci_resource_alignment_setup(char *str)
> > +{
> > + pci_set_resource_alignment_param(str, strlen(str));
> > +}
>
> Ditto.
I will remove it.
> > +
> > +static ssize_t pci_resource_alignment_show(struct bus_type *bus, char *buf)
> > +{
> > + return pci_get_resource_alignment_param(buf, PAGE_SIZE);
> > +}
> > +
> > +static ssize_t pci_resource_alignment_store(struct bus_type *bus,
> > + const char *buf, size_t count)
> > +{
> > + return pci_set_resource_alignment_param(buf, count);
>
> Check user input here?
pci_specified_resource_alignment outputs error message if it can't
parse the user input.
More checking is needed?
>
> > +}
> > +
> > +BUS_ATTR(resource_alignment, 0644, pci_resource_alignment_show,
> > + pci_resource_alignment_store);
> > +
> > index f20d553..b17f1aa 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -33,6 +33,63 @@ int pcie_mch_quirk;
> > EXPORT_SYMBOL(pcie_mch_quirk);
> >
> > #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 rounds up size to specified alignment.
> > + * Later on, the kernel will assign page-aligned memory resource back
> > + * to that device.
> > + */
> > +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;
>
> Use pci_specified_resource_alignment() directly here so it doesn't need
> to call it again below?
I think calling pci_is_reassigndev firstly is intuitive.
I'd not like to use pci_specified_resource_alignment() here.
Can you accept this?
> > +
> > + 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");
> > + return;
> > + }
> > +
> > + dev_info(&dev->dev, "Disabling device and release resources.\n");
> > + pci_disable_device(dev);
> > +
> > + align = pci_specified_resource_alignment(dev);
> > + for (i=0; i < PCI_NUM_RESOURCES; i++) {
>
> PCI_NUM_RESOURCES includes bus resources, it should be PCI_BRIDGE_RESOURCES.
I will use PCI_BRIDGE_RESOURCES here. For PCI-PCI bridge, I will
insert the code to free resource window. Please see below.
> > + r = &dev->resource[i];
> > + if (!(r->flags & IORESOURCE_MEM))
> > + continue;
> > + size = r->end - r->start + 1;
>
> Please use resource_size().
I will use resource_size().
> > + if (size < align) {
> > + size = align;
> > + dev_info(&dev->dev,
> > + "Rounding up size of resource #%d to %#llx.\n",
> > + i, (unsigned long long)size);
> > + }
> > + r->end = size - 1;
> > + r->start = 0;
> > +
> > + if (i < PCI_BRIDGE_RESOURCES) {
> > + pci_update_resource(dev, i);
> > + }
>
> The resource is not allocated, so it shouldn't call pci_update_resource(),
> especially with start=0.
My intention is syncing r->start to actual BAR.
pci_enable_resouces does not enable resource when resource in not allocated.
So this will not cause a issue.
> > + }
> > + /* need to disable bridge's resource window,
> > + * to enable the kernel to reassign new resource
> > + * window later on.
> > + */
> > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> > + (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
I will insert the following code.
for (i = PCI_BRIDGE_RESOURCES; i < PCI_NUM_RESOURCES; i++) {
r = &dev->resource[i];
if (!(r->flags & IORESOURCE_MEM))
continue;
r->end = resouce_size(r) - 1;
r->start = 0;
}
> > + pci_disable_bridge_window(dev);
> > + }
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, quirk_resource_alignment);
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
