| Message ID | 612e7f61c19e60019bb7829888342fda95fd36be.1624546532.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: smmuv1: Fixed stream matching register allocation | expand |
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@kernel.org> > Tested-by: Stefano Stabellini <sstabellini@kernel.org> > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > 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. Also, when sending series, please remember to create a cover letter and number each patch. Cheers,
Hi Julien, > On 30 Jun 2021, at 10:09 am, Julien Grall <julien@xen.org> 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@kernel.org> >> Tested-by: Stefano Stabellini <sstabellini@kernel.org> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> 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. > > Also, when sending series, please remember to create a cover letter and number each patch. > Ok. Regards, Rahul > Cheers, > > -- > Julien Grall
On 30/06/2021 18:49, Rahul Singh wrote: > Hi Julien, Hi, >> On 30 Jun 2021, at 10:09 am, Julien Grall <julien@xen.org> 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@kernel.org> >>> Tested-by: Stefano Stabellini <sstabellini@kernel.org> >>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>> --- >>> 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,
Hi Julien, > On 30 Jun 2021, at 7:00 pm, Julien Grall <julien@xen.org> wrote: > > > > On 30/06/2021 18:49, Rahul Singh wrote: >> Hi Julien, > > Hi, > >>> On 30 Jun 2021, at 10:09 am, Julien Grall <julien@xen.org> 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@kernel.org> >>>> Tested-by: Stefano Stabellini <sstabellini@kernel.org> >>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>>> --- >>>> 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
On 02/07/2021 10:54, Rahul Singh wrote: > Hi Julien, Hi Rahul, >> On 30 Jun 2021, at 7:00 pm, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 30/06/2021 18:49, Rahul Singh wrote: >>> Hi Julien, >> >> Hi, >> >>>> On 30 Jun 2021, at 10:09 am, Julien Grall <julien@xen.org> 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@kernel.org> >>>>> Tested-by: Stefano Stabellini <sstabellini@kernel.org> >>>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>>>> --- >>>>> 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; This sounds good to me. Cheers,
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); if (!smmu->smrs) return -ENOMEM;