Re: [PATCH v4 02/13] driver core: Set DMA ownership during driver bind/unbind

From: Lu Baolu <baolu.lu@linux.intel.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: baolu.lu@linux.intel.com, Joerg Roedel <joro@8bytes.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Christoph Hellwig <hch@infradead.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Dan Williams <dan.j.williams@intel.com>,
	rafael@kernel.org, Diana Craciun <diana.craciun@oss.nxp.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Eric Auger <eric.auger@redhat.com>, Liu Yi L <yi.l.liu@intel.com>,
	Jacob jun Pan <jacob.jun.pan@intel.com>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Stuart Yoder <stuyoder@gmail.com>,
	Laurentiu Tudor <laurentiu.tudor@nxp.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Li Yang <leoyang.li@nxp.com>, Dmitry Osipenko <digetx@gmail.com>,
	iommu@lists.linux-foundation.org, linux-pci@vger.kernel.org,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 02/13] driver core: Set DMA ownership during driver bind/unbind
Date: Thu, 23 Dec 2021 11:02:54 +0800	[thread overview]
Message-ID: <94e37c45-abc1-c682-5adf-1cc4b6887640@linux.intel.com> (raw)
In-Reply-To: <YcMeZlN3798noycN@kroah.com>

Hi Greg,

On 12/22/21 8:47 PM, Greg Kroah-Hartman wrote:
>> +
>> +	return ret;
>> +}
>> +
>> +static void device_dma_cleanup(struct device *dev, struct device_driver *drv)
>> +{
>> +	if (!dev->bus->dma_configure)
>> +		return;
>> +
>> +	if (!drv->suppress_auto_claim_dma_owner)
>> +		iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
>> +}
>> +
>>   static int really_probe(struct device *dev, struct device_driver *drv)
>>   {
>>   	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>> @@ -574,11 +601,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>>   	if (ret)
>>   		goto pinctrl_bind_failed;
>>   
>> -	if (dev->bus->dma_configure) {
>> -		ret = dev->bus->dma_configure(dev);
>> -		if (ret)
>> -			goto probe_failed;
>> -	}
>> +	if (device_dma_configure(dev, drv))
>> +		goto pinctrl_bind_failed;
> Are you sure you are jumping to the proper error path here?  It is not
> obvious why you changed this.

The error handling path in really_probe() seems a bit wrong. For
example,

  572         /* If using pinctrl, bind pins now before probing */
  573         ret = pinctrl_bind_pins(dev);
  574         if (ret)
  575                 goto pinctrl_bind_failed;

[...]

  663 pinctrl_bind_failed:
  664         device_links_no_driver(dev);
  665         devres_release_all(dev);
  666         arch_teardown_dma_ops(dev);
  667         kfree(dev->dma_range_map);
  668         dev->dma_range_map = NULL;
  669         driver_sysfs_remove(dev);
              ^^^^^^^^^^^^^^^^^^^^^^^^^
  670         dev->driver = NULL;
  671         dev_set_drvdata(dev, NULL);
  672         if (dev->pm_domain && dev->pm_domain->dismiss)
  673                 dev->pm_domain->dismiss(dev);
  674         pm_runtime_reinit(dev);
  675         dev_pm_set_driver_flags(dev, 0);
  676 done:
  677         return ret;

The driver_sysfs_remove() will be called even driver_sysfs_add() hasn't
been called yet. I can fix this in a separated patch if I didn't miss
anything.

Best regards,
baolu