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

[Alexey Kardashevskiy <aik@xxxxxxx>]

On 8/1/25 07:00, Xu Yilun wrote:
> > > > +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.

My rootport does not have any range (instead, it relies on C-bit in MMIO 
access to set T-bit). The device got just one (which is no use here as I 
understand).


> 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.

Oh. Yeah, this sounds right. So most of the setup needs to be done on 
the root port and not on the device (which only needs to enable the 
stream), which is not what the patch does. Or I got it wrong? Thanks,

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

--
Alexey