dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Vinod Koul <vkoul@kernel.org>
To: Dave Jiang <dave.jiang@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Logan Gunthorpe <logang@deltatee.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>
Subject: Re: [PATCH 1/5] dmaengine: Store module owner in dma_device struct
Date: Tue, 10 Dec 2019 15:23:27 +0530
Message-ID: <20191210095327.GA2536@vkoul-mobl> (raw)
In-Reply-To: <3ae58ea7-5cab-23f9-512f-bec30410ff6f@intel.com>

On 22-11-19, 14:42, Dave Jiang wrote:
> 
> 
> On 11/22/19 2:01 PM, Dan Williams wrote:
> > On Fri, Nov 22, 2019 at 12:56 PM Logan Gunthorpe <logang@deltatee.com> wrote:
> > > 
> > > 
> > > 
> > > On 2019-11-22 1:50 p.m., Dan Williams wrote:
> > > > On Fri, Nov 22, 2019 at 8:53 AM Dave Jiang <dave.jiang@intel.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 11/21/19 10:20 PM, Vinod Koul wrote:
> > > > > > On 14-11-19, 10:03, Logan Gunthorpe wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 2019-11-13 9:55 p.m., Vinod Koul wrote:
> > > > > > > > > But that's the problem. We can't expect our users to be "nice" and not
> > > > > > > > > unbind when the driver is in use. Killing the kernel if the user
> > > > > > > > > unexpectedly unbinds is not acceptable.
> > > > > > > > 
> > > > > > > > And that is why we review the code and ensure this does not happen and
> > > > > > > > behaviour is as expected
> > > > > > > 
> > > > > > > Yes, but the current code can kill the kernel when the driver is unbound.
> > > > > > > 
> > > > > > > > > > > I suspect this is less of an issue for most devices as they wouldn't
> > > > > > > > > > > normally be unbound while in use (for example there's really no reason
> > > > > > > > > > > to ever unbind IOAT seeing it's built into the system). Though, the fact
> > > > > > > > > > > is, the user could unbind these devices at anytime and we don't want to
> > > > > > > > > > > panic if they do.
> > > > > > > > > > 
> > > > > > > > > > There are many drivers which do modules so yes I am expecting unbind and
> > > > > > > > > > even a bind following that to work
> > > > > > > > > 
> > > > > > > > > Except they will panic if they unbind while in use, so that's a
> > > > > > > > > questionable definition of "work".
> > > > > > > > 
> > > > > > > > dmaengine core has module reference so while they are being used they
> > > > > > > > won't be removed (unless I complete misread the driver core behaviour)
> > > > > > > 
> > > > > > > Yes, as I mentioned in my other email, holding a module reference does
> > > > > > > not prevent the driver from being unbound. Any driver can be unbound by
> > > > > > > the user at any time without the module being removed.
> > > > > > 
> > > > > > That sounds okay then.
> > > > > 
> > > > > I'm actually glad Logan is putting some work in addressing this. I also
> > > > > ran into the same issue as well dealing with unbinds on my new driver.
> > > > 
> > > > This was an original mistake of the dmaengine implementation that
> > > > Vinod inherited. Module pinning is distinct from preventing device
> > > > unbind which ultimately can't be prevented. Longer term I think we
> > > > need to audit dmaengine consumers to make sure they are prepared for
> > > > the driver to be removed similar to how other request based drivers
> > > > can gracefully return an error status when the device goes away rather
> > > > than crashing.

Right finally wrapping my head of static dmaengine devices! we can
indeed have devices going away, but me wondering why this should be
handled in subsystems! Should the driver core not be doing this instead?
Would it be not a safe behaviour for unplug to unload the driver and
thus give a chance to unbind from subsystems too...


> > > Yes, but that will be a big project because there are a lot of drivers.
> > 
> > Oh yes, in fact I think it's something that can only reasonably be
> > considered for new consumers.
> > 
> > > But I think the dmaengine common code needs to support removal properly,
> > > which essentially means changing how all the drivers allocate and free
> > > their structures, among other things.
> > > 
> > > The one saving grace is that most of the drivers are for SOCs which
> > > can't be physically removed and there's really no use-case for the user
> > > to call unbind.

yeah only a small set of drivers would need this for now!

> > Yes, the SOC case is not so much my concern as the generic offload use
> > cases, especially if those offloads are in a similar hotplug domain as
> > a cpu.
> 
> It becomes a bigger issue when "channels" can be reconfigured and can come
> and go in a hot plug fashion.

-- 
~Vinod

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-22 21:46 [PATCH 0/5] PLX Switch DMA Engine Driver Logan Gunthorpe
2019-10-22 21:46 ` [PATCH 1/5] dmaengine: Store module owner in dma_device struct Logan Gunthorpe
2019-11-09 17:18   ` Vinod Koul
2019-11-11 16:50     ` Logan Gunthorpe
2019-11-12  5:56       ` Vinod Koul
2019-11-12 16:45         ` Logan Gunthorpe
2019-11-14  4:55           ` Vinod Koul
2019-11-14 17:03             ` Logan Gunthorpe
2019-11-22  5:20               ` Vinod Koul
2019-11-22 16:53                 ` Dave Jiang
2019-11-22 20:50                   ` Dan Williams
2019-11-22 20:56                     ` Logan Gunthorpe
2019-11-22 21:01                       ` Dan Williams
2019-11-22 21:42                         ` Dave Jiang
2019-12-10  9:53                           ` Vinod Koul [this message]
2019-12-10 17:39                             ` Logan Gunthorpe
2019-10-22 21:46 ` [PATCH 2/5] dmaengine: Call module_put() after device_free_chan_resources() Logan Gunthorpe
2019-10-22 21:46 ` [PATCH 3/5] dmaengine: plx-dma: Introduce PLX DMA engine PCI driver skeleton Logan Gunthorpe
2019-11-09 17:35   ` Vinod Koul
2019-11-11 17:50     ` Logan Gunthorpe
2019-11-12  6:09       ` Vinod Koul
2019-11-12 17:22         ` Logan Gunthorpe
2019-10-22 21:46 ` [PATCH 4/5] dmaengine: plx-dma: Implement hardware initialization and cleanup Logan Gunthorpe
2019-10-22 21:46 ` [PATCH 5/5] dmaengine: plx-dma: Implement descriptor submission Logan Gunthorpe
2019-11-09 17:40   ` Vinod Koul
2019-11-11 18:11     ` Logan Gunthorpe
2019-12-16 19:01 [PATCH 0/5] Support hot-unbind in IOAT Logan Gunthorpe
2019-12-16 19:01 ` [PATCH 1/5] dmaengine: Store module owner in dma_device struct Logan Gunthorpe

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191210095327.GA2536@vkoul-mobl \
    --to=vkoul@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=logang@deltatee.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git