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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 30 Jun 2021, at 7:00 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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?

Ok. I will modify the code as per your request as below . I will use 
devm_kcalloc(..) as this will be more coherent.

diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index da2cd457d7..658c40433c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -149,7 +149,8 @@ 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)
+#define devm_kcalloc(dev, n, size, flags)                      \
+       _xzalloc_array(size, sizeof(void *), n)
 
 static void __iomem *devm_ioremap_resource(struct device *dev,
                                           struct resource *res)
@@ -2222,7 +2223,8 @@ 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 = kzalloc_array(sizeof(*smmu->smrs), size, 
GFP_KERNEL);
+               smmu->smrs = devm_kcalloc(smmu->dev, size, sizeof(*smmu->smrs),
+                                                               GFP_KERNEL);
                if (!smmu->smrs)
                        return -ENOMEM;

Regards,
Rahul

> 
> Cheers,
> 
> -- 
> Julien Grall