Re: [PATCH 1/3 v2] drivers/bus: Added Freescale Management Complex APIs

[German Rivera]

On 09/23/2014 07:49 PM, Kim Phillips wrote:
> On Fri, 19 Sep 2014 17:49:39 -0500
> "J. German Rivera" <German.Rivera@xxxxxxxxxxxxx> wrote:
>
> > +int mc_get_version(struct fsl_mc_io *mc_io, struct mc_version *mc_ver_info)
> ...
> > +	err = mc_send_command(mc_io, &cmd);
> > +	if (err)
> > +		return err;
> > +
> > +		DPMNG_RSP_GET_VERSION(cmd, mc_ver_info);
>
> alignment
>
> > +int dpmng_load_aiop(struct fsl_mc_io *mc_io,
> > +		    int aiop_tile_id, uint8_t *img_addr, int img_size)
> > +{
> > +	struct mc_command cmd = { 0 };
> > +	uint64_t img_paddr = virt_to_phys(img_addr);
>
> Direct use of virt_to_phys by drivers is deprecated; this code needs
> to map the i/o space via the DMA API.  This is in order to handle
> situations where e.g., the device sitting behind an IOMMU.  See
> Documentation/DMA-API* for more info.
Ok, we will make this change in the v3 respin.

> > +/**
> > + * 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():

int dprc_get_irq_status(struct fsl_mc_io *mc_io, uint16_t dprc_handle,
			uint8_t irq_index, uint32_t *status)
{
	struct mc_command cmd = { 0 };
	int err;

	cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_IRQ_STATUS,
					  DPRC_CMDSZ_GET_IRQ_STATUS,
					  MC_CMD_PRI_LOW, dprc_handle);

	DPRC_CMD_GET_IRQ_STATUS(cmd, irq_index);
	err = mc_send_command(mc_io, &cmd);
	if (!err)
		DPRC_RSP_GET_IRQ_STATUS(cmd, *status);

	return err;
}

int dprc_clear_irq_status(struct fsl_mc_io *mc_io, uint16_t dprc_handle,
			  uint8_t irq_index, uint32_t status)
{
	struct mc_command cmd = { 0 };

	cmd.header = mc_encode_cmd_header(DPRC_CMDID_CLEAR_IRQ_STATUS,
					  DPRC_CMDSZ_CLEAR_IRQ_STATUS,
					  MC_CMD_PRI_LOW, dprc_handle);

	DPRC_CMD_CLEAR_IRQ_STATUS(cmd, status, irq_index);
	return mc_send_command(mc_io, &cmd);
}

> > +	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.

> > +	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.

> > +}
> > +EXPORT_SYMBOL_GPL(fsl_destroy_mc_io);
> > +
> > +static int mc_status_to_error(enum mc_cmd_status status)
> > +{
> > +	static const int mc_status_to_error_map[] = {
> > +		[MC_CMD_STATUS_OK] = 0,
> > +		[MC_CMD_STATUS_AUTH_ERR] = -EACCES,
> > +		[MC_CMD_STATUS_NO_PRIVILEGE] = -EPERM,
> > +		[MC_CMD_STATUS_DMA_ERR] = -EIO,
> > +		[MC_CMD_STATUS_CONFIG_ERR] = -ENXIO,
> > +		[MC_CMD_STATUS_TIMEOUT] = -ETIMEDOUT,
> > +		[MC_CMD_STATUS_NO_RESOURCE] = -ENAVAIL,
> > +		[MC_CMD_STATUS_NO_MEMORY] = -ENOMEM,
> > +		[MC_CMD_STATUS_BUSY] = -EBUSY,
> > +		[MC_CMD_STATUS_UNSUPPORTED_OP] = -ENOTSUPP,
> > +		[MC_CMD_STATUS_INVALID_STATE] = -ENODEV,
> > +	};
> > +
> > +	if (WARN_ON(status >= ARRAY_SIZE(mc_status_to_error_map)))
> > +		return -EINVAL;
> > +	return mc_status_to_error_map[status];
> > +}
>
> great - CONFIG_ERR is now -ENXIO instead of -EINVAL, so at least
> it's now mutually exclusive, but it doesn't handle
> MC_CMD_STATUS_READY - should it?
No. MC_CMD_STATUS_READY does not need to be seen by mc_send_command()
callers. It is only used to know when to stop polling the MC for
command completion.

> > +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

What do you mean by nesting in this case?

> > +	/*
> > +	 * 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.

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