Date:	Wed, 24 Sep 2014 22:40:06 -0500
From:	Kim Phillips <kim.phillips@...escale.com>
To:	German Rivera <German.Rivera@...escale.com>
CC:	<gregkh@...uxfoundation.org>, <arnd@...db.de>,
	<linux-kernel@...r.kernel.org>, <stuart.yoder@...escale.com>,
	<scottwood@...escale.com>, <agraf@...e.de>,
	<linuxppc-release@...ux.freescale.net>,
	Erez Nir-RM30794 <nir.erez@...escale.com>
Subject: Re: [PATCH 1/3 v2] drivers/bus: Added Freescale Management Complex
 APIs

On Wed, 24 Sep 2014 21:23:59 -0500
German Rivera <German.Rivera@...escale.com> wrote:

> On 09/23/2014 07:49 PM, Kim Phillips wrote:
> > On Fri, 19 Sep 2014 17:49:39 -0500
> > "J. German Rivera" <German.Rivera@...escale.com> wrote:
> >
> >> + * Delay in microseconds between polling iterations while
> >> + * waiting for MC command completion
> >> + */
> >> +#define MC_CMD_COMPLETION_POLLING_INTERVAL_USECS    500	/* 0.5 ms */
> >> +
> >> +int __must_check fsl_create_mc_io(struct device *dev,
> >> +				  phys_addr_t mc_portal_phys_addr,
> >> +				  uint32_t mc_portal_size,
> >> +				  uint32_t flags, struct fsl_mc_io **new_mc_io)
> >> +{
> >> +	struct fsl_mc_io *mc_io;
> >> +	void __iomem *mc_portal_virt_addr;
> >> +	struct resource *res;
> >> +
> >> +	mc_io = devm_kzalloc(dev, sizeof(*mc_io), GFP_KERNEL);
> >> +	if (mc_io == NULL)
> >> +		return -ENOMEM;
> >> +
> >> +	mc_io->dev = dev;
> >> +	mc_io->flags = flags;
> >> +	mc_io->portal_phys_addr = mc_portal_phys_addr;
> >> +	mc_io->portal_size = mc_portal_size;
> >> +	if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_INT_HANDLERS)
> >> +		spin_lock_init(&mc_io->spinlock);
> >
> > I'm confused - this patseries doesn't register an interrupt handler,
> > so this can't be true (or it's premature if it will, in which case
> > it should be left out for now).
> >
> > However, if somehow users of this code register an IRQ handler for
> > the portal (I don't see any users to tell how they get the IRQ line
> > either?), then it's up to them to establish mutual exclusion rules
> > for access, among themselves.  Unless you think they will be calling
> > mc_send_command from h/w IRQ context, in which case I'd reconsider
> > that assumption because send_command looks like it'd take too long
> > to get an answer from the h/w - IRQ handlers should just ack the h/w
> > IRQ, and notify the scheduler that the driver has work to do (in s/w
> > IRQ context perhaps).
> >
> Although not included in this patch series, there are cases in
> subsequent patch series, in which mc_send_command() will need to be
> called from interrupt context.
> 
> For example, the dprc_get_irq_status() needs to be called from the DPRC
> ISR to determine the actual cause of the interrupt. Also, to clear
> the interrupt the dprc_clear_irq_status() needs to be called from the
> ISR. Both od these functions call mc_send_command():

yet mc_send_command itself has the capability to trigger a h/w IRQ,
no?  So how is that expected to work, given h/w IRQ handlers run
with IRQs turned off?

> >> +	else if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_THREADS)
> >> +		mutex_init(&mc_io->mutex);
> >
> > I'd assume SHARED_BY_THREADS to always be true in linux.
> >
> Not, if mc_send_command() is called from interrupt context, as
> explained above. However, since this patch series does not include
> any interrupt handlers, we can remove the
> FSL_MC_PORTAL_SHARED_BY_INT_HANDLERS flag and the associated spinlock,
> from this patch series.
> 
> >> +	res = devm_request_mem_region(dev,
> >> +				      mc_portal_phys_addr,
> >> +				      mc_portal_size,
> >> +				      "mc_portal");
> >> +	if (res == NULL) {
> >> +		dev_err(dev,
> >> +			"devm_request_mem_region failed for MC portal %#llx\n",
> >> +			mc_portal_phys_addr);
> >> +		return -EBUSY;
> >> +	}
> >> +
> >> +	mc_portal_virt_addr = devm_ioremap_nocache(dev,
> >> +						   mc_portal_phys_addr,
> >> +						   mc_portal_size);
> >> +	if (mc_portal_virt_addr == NULL) {
> >> +		dev_err(dev,
> >> +			"devm_ioremap_nocache failed for MC portal %#llx\n",
> >> +			mc_portal_phys_addr);
> >> +		return -ENXIO;
> >> +	}
> >> +
> >> +	mc_io->portal_virt_addr = mc_portal_virt_addr;
> >> +	*new_mc_io = mc_io;
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(fsl_create_mc_io);
> >> +
> >> +void fsl_destroy_mc_io(struct fsl_mc_io *mc_io)
> >> +{
> >> +	if (WARN_ON(mc_io->portal_virt_addr == NULL))
> >> +		return;
> >
> > this is unnecessary - you'll get the stack trace anyway, and users
> > calling destroy on a not successfully created mc_io object should
> > not get the luxury of maybe being able to continue after the stack
> > trace, after possibly leaking memory.
> Ok, I'ĺl remove this WARN_ON in the v3 respin.

subsequent code will cause a stack trace when dereferencing
portal_virt_addr anyway, so the if statement isn't needed at all.

> >> +	mc_io->portal_virt_addr = NULL;
> >> +	devm_kfree(mc_io->dev, mc_io);
> >
> > like I said before, there's really no point in clearing something
> > out right before it's freed.
> >
> I disagree. This can help detect cases of double-freeing.

?  freeing NULL does nothing - it just returns - which doesn't help
detect anything.  What's more, the kernel has a memory debugging
infrastructure that detects double freeing of the same object.

> >> +int mc_send_command(struct fsl_mc_io *mc_io, struct mc_command *cmd)
> >> +{
> >> +	enum mc_cmd_status status;
> >> +	int error;
> >> +	unsigned long irqsave_flags = 0;
> >> +	unsigned long jiffies_until_timeout =
> >> +	    jiffies + MC_CMD_COMPLETION_TIMEOUT_JIFFIES;
> >> +
> >> +	/*
> >> +	 * Acquire lock depending on mc_io flags:
> >> +	 */
> >> +	if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_INT_HANDLERS) {
> >> +		if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_THREADS)
> >> +			spin_lock_irqsave(&mc_io->spinlock, irqsave_flags);
> >> +		else
> >> +			spin_lock(&mc_io->spinlock);
> >> +	} else if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_THREADS) {
> >> +		mutex_lock(&mc_io->mutex);
> >> +	}
> >
> > again, I think we need to drop the coming from h/w IRQ context here
> > (SHARED_BY_INT_HANDLERS); there's no IRQ handlers in this
> > patchseries, and calling this function from an IRQ handler would be
> > prohibitively wasteful, guessing by the udelay and timeout values
> > below.
> >
> > Can we just mutex_lock for now, and unconditionally (no
> > SHARED_BY_THREADS check), to protect from nesting?
> >
> I would still prefer to keep the SHARED_BY_THREADS flag, to give option 
> of not doing any locking, in cases where the portal used in 
> mc_send_command() is not shared among concurrent callers

how can you guarantee there won't be concurrent callers?  The linux
kernel is multithreaded.

> What do you mean by nesting in this case?

when a thread gets interrupted by another thread, and/or another
IRQ: this would cause an unrecoverable race condition.

> >> +	/*
> >> +	 * Wait for response from the MC hardware:
> >> +	 */
> >> +	for (;;) {
> >> +		status = mc_read_response(mc_io->portal_virt_addr, cmd);
> >> +		if (status != MC_CMD_STATUS_READY)
> >> +			break;
> >> +
> >> +		/*
> >> +		 * TODO: When MC command completion interrupts are supported
> >> +		 * call wait function here instead of udelay()
> >> +		 */
> >> +		udelay(MC_CMD_COMPLETION_POLLING_INTERVAL_USECS);
> >
> > this pauses any caller for 0.5ms on every successful command
> > write.  Can the next submission of the patchseries wait until
> > completion IRQs are indeed supported, since both that and the above
> > locking needs to be resolved?
> >
> No. Interrupt handlers will come in a later patch series as they are
> not needed for using the basic MC functionality.

meanwhile unnecessarily udelaying kernel threads for .5ms upsets
basic kernel functionality :)  Would using the kernel's
wait_for_completion API be a good compromise here?  See
include/linux/completion.h.

Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists