RE: [PATCH 3/5] iommu/msm: Add support for generic master bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@xxxxxxxxxxxxxx]
> Sent: Thursday, August 13, 2015 12:42 AM
> To: Sricharan R
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx;
> robin.murphy@xxxxxxx; robdclark@xxxxxxxxx; joro@xxxxxxxxxx;
> srinivas.kandagatla@xxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx;
> Will.Deacon@xxxxxxx; stepanm@xxxxxxxxxxxxxx; treding@xxxxxxxxxx
> Subject: Re: [PATCH 3/5] iommu/msm: Add support for generic master
> bindings
> 
> On 08/12/2015 07:47 AM, Sricharan R wrote:
> > @@ -702,6 +703,44 @@ static void print_ctx_regs(void __iomem *base,
> int ctx)
> >   	       GET_PRRR(base, ctx), GET_NMRR(base, ctx));
> >   }
> >
> > +static void insert_iommu_master(struct device *dev,
> > +				struct msm_iommu_dev *iommu,
> > +				struct of_phandle_args *spec)
> > +{
> > +	struct msm_iommu_ctx_dev *master;
> > +	int sid;
> > +
> > +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> 
> This is called with irqs disabled, but it's not GFP_ATOMIC. Please test
with
> DEBUG_ATOMIC_SLEEP=y.

    Ok. I will have to adjust the locking here. It should be granular only
for the list manipulation.
> 
> > +	master->of_node = dev->of_node;
> > +	list_add(&master->list, &iommu->ctx_list);
> > +
> > +	for (sid = 0; sid < spec->args_count; sid++)
> > +		master->mids[sid] = spec->args[sid];
> > +
> > +	master->num_mids = spec->args_count; }
> > +
> > +static int qcom_iommu_of_xlate(struct device *dev,
> > +			       struct of_phandle_args *spec) {
> > +	struct msm_iommu_dev *iommu;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&msm_iommu_lock, flags);
> > +	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
> > +		if (iommu->dev->of_node == spec->np)
> > +			break;
> > +	}
> 
> The braces are unnecessary here.
 Ok, will remove this.
> 
> > +
> > +	if (!iommu || (iommu->dev->of_node != spec->np))
> 
> Please remove extraneous parentheses.
> 
 Ok.

> > +		return -ENODEV;
> > +
> > +	insert_iommu_master(dev, iommu, spec);
> > +	spin_unlock_irqrestore(&msm_iommu_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> >   irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
> >   {
> >   	struct msm_iommu_dev *iommu = dev_id; @@ -737,7 +776,7 @@
> fail:
> >   	return 0;
> >   }
> >
> > -static const struct iommu_ops msm_iommu_ops = {
> > +static struct iommu_ops msm_iommu_ops = {
> 
> Is there a reason why we can't make of_iommu_set_ops() take a const ops
> pointer?
> 
  Hmm right. of_iommu_set_ops is the one that needs change. I will add a
separate
  patch to correct that.

Regards,
 Sricharan

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux