Re: [PATCH 0/3] dmaengine: Stear users towards dma_request_slave_chan()

[Vinod Koul]

Hi Peter,

On 05-02-20, 10:10, Peter Ujfalusi wrote:
> Vinod,
> 
> On 05/02/2020 6.43, Vinod Koul wrote:
> > On 04-02-20, 13:21, Andy Shevchenko wrote:
> >> On Tue, Feb 4, 2020 at 8:21 AM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> >>>
> >>> On 03-02-20, 12:37, Andy Shevchenko wrote:
> >>>> On Mon, Feb 3, 2020 at 12:32 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
> >>>>
> >>>>> dma_request_slave_channel_reason() no longer have user in mainline, it
> >>>>> can be removed.
> >>>>>
> >>>>> Advise users of dma_request_slave_channel() and
> >>>>> dma_request_slave_channel_compat() to move to dma_request_slave_chan()
> >>>>
> >>>> How? There are legacy ARM boards you have to care / remove before.
> >>>> DMAengine subsystem makes a p*s off decisions without taking care of
> >>>> (I'm talking now about dma release callback, for example) end users.
> >>>
> >>> Can you elaborate issue you are seeing with dma_release callback?
> >>
> >>
> >> [    7.980381] intel-lpss 0000:00:1e.3: WARN: Device release is not
> >> defined so it is not safe to unbind this driver while in use
> > 
> > Yes that is expected but is not valid in your case.
> 
> In which case it is valid?

It is valid for cases where device can be hotplugged. We didnt handle
that very well earlier. TBH we never had a reason as most of the
embedded cases that is not really doable.

> > Anyway this will be turned off before the release.
> 
> Looking at the commit which added it and I still don't get the point.
> If any of the channel is in use then we should not allow the DMA driver
> to go away at all.

Not really, if the device is already gone, we cant do much about it. We
have to handle that gracefully rather than oopsing

The important part is that the device is gone. Think about a device on
PCI card which is yanked off or a USB device unplugged. Device is
already gone, you can't communicate with it anymore. So all we can do is
handle the condition and exit, hence the new method to let driver know.

> Imho there should be a function to check if we can proceed with the
> .remove of the driver and fail it if any of the channels are in use.
> 
> Hrm, base/dd.c __device_release_driver() does not check the .remove's
> return value, so it can not fail.
> 
> What is expected if the .remove returns with OK but we still have
> channels in use?
> 
> After the remove all sorts of things got yanked which might makes the
> still in use channels cause issues down the road.
> 
> I'm curious why it is a good thing to remotely try to support unbind
> when the driver is in use.
> It is like one wants to support ext4 removal even when your rootfs is ext4.
> 
> I think krefing the DMA driver for channel request/release is just fine,
> if user wants to break the system we should not assist...
> 
> >> It's not limited to that driver, but actually all I'm maintaining.
> >>
> >> Users are not happy!
> >>
> >> -- 
> >> With Best Regards,
> >> Andy Shevchenko
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
~Vinod