Mario Limonciello <mario.limonciello@xxxxxxx> writes:
> On 9/5/2025 1:48 PM, Nathan Lynch via B4 Relay wrote:
>> +static int sdxi_pci_map(struct sdxi_dev *sdxi)
>> +{
>> + struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> + int bars, ret;
>> +
>> + bars = 1 << MMIO_CTL_REGS_BAR | 1 << MMIO_DOORBELL_BAR;
>> + ret = pcim_iomap_regions(pdev, bars, SDXI_DRV_NAME);
>> + if (ret) {
>> + sdxi_err(sdxi, "pcim_iomap_regions failed (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + sdxi->dbs_bar = pci_resource_start(pdev, MMIO_DOORBELL_BAR);
>> +
>> + /* FIXME: pcim_iomap_table may return NULL, and it's deprecated. */
>> + sdxi->ctrl_regs = pcim_iomap_table(pdev)[MMIO_CTL_REGS_BAR];
>> + sdxi->dbs = pcim_iomap_table(pdev)[MMIO_DOORBELL_BAR];
>> + if (!sdxi->ctrl_regs || !sdxi->dbs) {
>> + sdxi_err(sdxi, "pcim_iomap_table failed\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
>> +{
>> + struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +
>> + pcim_iounmap(pdev, sdxi->ctrl_regs);
>> + pcim_iounmap(pdev, sdxi->dbs);
>> +}
>> +
>> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
>> +{
>> + struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> + struct device *dev = &pdev->dev;
>> + int dma_bits = 64;
>> + int ret;
>> +
>> + ret = pcim_enable_device(pdev);
>> + if (ret) {
>> + sdxi_err(sdxi, "pcim_enbale_device failed\n");
>> + return ret;
>> + }
>> +
>> + pci_set_master(pdev);
>
> Does pci_set_master() need to come before dma_set_mask_and_coherent()
> and sdxi_pci_map()?
Yes, I think so. I'll audit this code for conformance to the
initialization sequence described in Documentation/pci/pci.rst.
>
> If so; there should be an error handling path that does
> pci_clear_master().
Right.
>
>> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
>> + if (ret) {
>> + sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
>> + return ret;
>> + }
>> +
>> + ret = sdxi_pci_map(sdxi);
>> + if (ret) {
>> + sdxi_err(sdxi, "failed to map device IO resources\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void sdxi_pci_exit(struct sdxi_dev *sdxi)
>> +{
>> + sdxi_pci_unmap(sdxi);
>
> I think you need a pci_clear_master() here.
OK, will fix.
