Re: [PATCH 08/11] PCI/IDE: Add IDE establishment helpers

[Xu Yilun <yilun.xu@linux.intel.com>]

> > > +static void __pci_ide_stream_setup(struct pci_dev *pdev, struct
> > > pci_ide *ide)
> > > +{
> > > +    int pos;
> > > +    u32 val;
> > > +
> > > +    pos = sel_ide_offset(pdev->sel_ide_cap, ide->stream_id,
> > > +                 pdev->nr_ide_mem);
> > > +
> > > +    val = FIELD_PREP(PCI_IDE_SEL_RID_1_LIMIT_MASK, ide->devid_end);
> > > +    pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_1, val);
> > > +
> > > +    val = FIELD_PREP(PCI_IDE_SEL_RID_2_VALID, 1) |
> > > +          FIELD_PREP(PCI_IDE_SEL_RID_2_BASE_MASK, ide->devid_start) |
> > > +          FIELD_PREP(PCI_IDE_SEL_RID_2_SEG_MASK, ide->domain);
> > > +    pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_2, val);
> > > +
> > > +    for (int i = 0; i < ide->nr_mem; i++) {
> > 
> > 
> > This needs to test that (pdev->nr_ide_mem >= ide->nr_mem), easy to miss
> > especially when PCI_IDE_SETUP_ROOT_PORT. Thanks,

Yes, but nr_ide_mem is limited HW resource and may easily smaller than
device memory region number. In this case, maybe we have to merge the
memory regions into one big range.

> > 
> > 
> > 
> > > +        val = FIELD_PREP(PCI_IDE_SEL_ADDR_1_VALID, 1) |
> > > +              FIELD_PREP(PCI_IDE_SEL_ADDR_1_BASE_LOW_MASK,
> > > +                 lower_32_bits(ide->mem[i].start) >>
> > > +                     PCI_IDE_SEL_ADDR_1_BASE_LOW_SHIFT) |
> > > +              FIELD_PREP(PCI_IDE_SEL_ADDR_1_LIMIT_LOW_MASK,
> > > +                 lower_32_bits(ide->mem[i].end) >>
> > > +                     PCI_IDE_SEL_ADDR_1_LIMIT_LOW_SHIFT);
> > > +        pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_1(i), val);
> > > +
> > > +        val = upper_32_bits(ide->mem[i].end);
> > > +        pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_2(i), val);
> > > +
> > > +        val = upper_32_bits(ide->mem[i].start);
> > > +        pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_3(i), val);
> > > +    }
> > > +}
> > > +
> > > +/*
> > > + * Establish IDE stream parameters in @pdev and, optionally, its
> > > root port
> > > + */
> > > +int pci_ide_stream_setup(struct pci_dev *pdev, struct pci_ide *ide,
> > > +             enum pci_ide_flags flags)
> > > +{
> > > +    struct pci_host_bridge *hb = pci_find_host_bridge(pdev->bus);
> > > +    struct pci_dev *rp = pcie_find_root_port(pdev);
> > > +    int mem = 0, rc;
> > > +
> > > +    if (ide->stream_id < 0 || ide->stream_id > U8_MAX) {
> > > +        pci_err(pdev, "Setup fail: Invalid stream id: %d\n",
> > > ide->stream_id);
> > > +        return -ENXIO;
> > > +    }
> > > +
> > > +    if (test_and_set_bit_lock(ide->stream_id, hb->ide_stream_ids)) {
> > > +        pci_err(pdev, "Setup fail: Busy stream id: %d\n",
> > > +            ide->stream_id);
> > > +        return -EBUSY;
> > > +    }
> > > +
> > > +    ide->name = kasprintf(GFP_KERNEL, "stream%d:%s", ide->stream_id,
> > > +                  dev_name(&pdev->dev));
> > > +    if (!ide->name) {
> > > +        rc = -ENOMEM;
> > > +        goto err_name;
> > > +    }
> > > +
> > > +    rc = sysfs_create_link(&hb->dev.kobj, &pdev->dev.kobj, ide->name);
> > > +    if (rc)
> > > +        goto err_link;
> > > +
> > > +    for (mem = 0; mem < ide->nr_mem; mem++)
> > > +        if (!__request_region(&hb->ide_stream_res, ide->mem[mem].start,
> > > +                      range_len(&ide->mem[mem]), ide->name,
> > > +                      0)) {
> > > +            pci_err(pdev,
> > > +                "Setup fail: stream%d: address association conflict
> > > [%#llx-%#llx]\n",
> > > +                ide->stream_id, ide->mem[mem].start,
> > > +                ide->mem[mem].end);
> > > +
> > > +            rc = -EBUSY;
> > > +            goto err;
> > > +        }
> > > +
> > > +    __pci_ide_stream_setup(pdev, ide);
> > > +    if (flags & PCI_IDE_SETUP_ROOT_PORT)
> > > +        __pci_ide_stream_setup(rp, ide);
> 
> Oh, when we do this, the root port gets the same devid_start/end as the
> device which is not correct, what should be there, the rootport bdfn? Need

"Indicates the lowest/highest value RID in the range
associated with this Stream ID at the IDE *Partner* Port"

My understanding is that device should fill the RP bdfn, and the RP
should fill the device bdfn for RID association registers. Same for Addr
association registers.

Thanks,
Yilun

> to dig that but PCI_IDE_SETUP_ROOT_PORT should detect that it is a root
> port. Thanks,
>