Re: [PATCH] xen/arm: smmuv1: Fixed stream matching register allocation

[Julien Grall <julien@xxxxxxx>]

On 30/06/2021 18:49, Rahul Singh wrote:
> Hi Julien,
Hi,

> > On 30 Jun 2021, at 10:09 am, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Rahul,
> >
> > On 25/06/2021 17:37, Rahul Singh wrote:
> > > SMR allocation should be based on the number of supported stream
> > > matching register for each SMMU device.
> > > Issue introduced by commit 5e08586afbb90b2e2d56c175c07db77a4afa873c
> > > when backported the patches from Linux to XEN to fix the stream match
> > > conflict issue when two devices have the same stream-id.
> > > Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Tested-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > ---
> > >   xen/drivers/passthrough/arm/smmu.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > index d9a3a0cbf6..da2cd457d7 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -149,6 +149,7 @@ typedef enum irqreturn irqreturn_t;
> > >   #define kzalloc(size, flags)          _xzalloc(size, sizeof(void *))
> > >   #define devm_kzalloc(dev, size, flags)        _xzalloc(size, sizeof(void *))
> > >   #define kmalloc_array(size, n, flags) _xmalloc_array(size, sizeof(void *), n)
> > > +#define kzalloc_array(size, n, flags)  _xzalloc_array(size, sizeof(void *), n)
> > >     static void __iomem *devm_ioremap_resource(struct device *dev,
> > >                                            struct resource *res)
> > > @@ -2221,7 +2222,7 @@ static int arm_smmu_device_cfg_probe(struct 
> > > arm_smmu_device *smmu)
> > >                 smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
> > >                 /* Zero-initialised to mark as invalid */
> > > -               smmu->smrs = devm_kzalloc(smmu->dev, sizeof(*smmu->smrs), 
> > > GFP_KERNEL);
> > > +               smmu->smrs = kzalloc_array(sizeof(*smmu->smrs), size, 
> > > GFP_KERNEL);
> > I noticed this is already in... However, I am a bit puzzled into why this was 
> > switched devm_kzalloc() to kzalloc_array(). This doesn't matter for Xen as they 
> > are just wrappers to x*alloc() but a mention in the commit message would have 
> > been useful.
> Yes we can use the devm_kzalloc(..) but then we have to pass 
> (sizeof(*smmu->smrs) * size ) as size argument to devm_kzalloc(..)
> I thought for better code readability I will use kzalloc_array() as the 
> function name suggests we are allocating memory for an array.
My point is devm_k*alloc() and k*alloc() are quite different on the 
paper. One will allocate memory for a given device while the other is 
unknown memory.
It would have been better to call the function devm_kzalloc_array() to 
keep to keep the code coherent. Can you please send a patch to make the 
switch?
Cheers,

--
Julien Grall