|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: smmu-v3: Fix MISRA C 2012 Rule 1.3 violations
On 29/06/2022 15:10, Bertrand Marquis wrote: Hi, Hi Bertrand, In fact the patch was committed before we started this discussion as Rahul R-b was enough. It was probably merged a bit too fast. When there are multiple maintainers responsible for the code, I tend to prefer to wait a bit just in case there are extra comments. But I would still be interested to have other maintainers view on this. Rahul and you are the official maintainers for that code. I think Stefano and I are only CCed because we maintain the Arm code (get_maintainers.pl doesn't automatically remove maintainers from upper directory). On 29 Jun 2022, at 10:03, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote: Hi Xenia,On 29 Jun 2022, at 09:55, xenia <burzalodowa@xxxxxxxxx> wrote: Hi Bertrand, On 6/29/22 10:24, Bertrand Marquis wrote:Hi Xenia,On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@xxxxxxxxx> wrote: The expression 1 << 31 produces undefined behaviour because the type of integer constant 1 is (signed) int and the result of shifting 1 by 31 bits is not representable in the (signed) int type. Change the type of 1 to unsigned int by adding the U suffix. Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx> --- Q_OVERFLOW_FLAG has already been fixed in upstream kernel code. For GBPA_UPDATE I will submit a patch. xen/drivers/passthrough/arm/smmu-v3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c index 1e857f915a..f2562acc38 100644 --- a/xen/drivers/passthrough/arm/smmu-v3.c +++ b/xen/drivers/passthrough/arm/smmu-v3.c @@ -338,7 +338,7 @@ static int platform_get_irq_byname_optional(struct device *dev, #define CR2_E2H (1 << 0) #define ARM_SMMU_GBPA 0x44 -#define GBPA_UPDATE (1 << 31) +#define GBPA_UPDATE (1U << 31) #define GBPA_ABORT (1 << 20) #define ARM_SMMU_IRQ_CTRL 0x50 @@ -410,7 +410,7 @@ static int platform_get_irq_byname_optional(struct device *dev, #define Q_IDX(llq, p) ((p) & ((1 << (llq)->max_n_shift) - 1)) #define Q_WRP(llq, p) ((p) & (1 << (llq)->max_n_shift))Could also make sense to fix those 2 to be coherent.According to the spec, the maximum value that max_n_shift can take is 19. Hence, 1 << (llq)->max_n_shift cannot produce undefined behavior.I agree with that but my preference would be to not rely on deep analysis on the code for those kind of cases and use 1U whenever possible. What other maintainers think on this ? In general, I prefer if we use 1U (or 1UL/1ULL) so it is clearer that the code is correct and consistent (I always find odd when we use 1U << 31 but 1 << 20). In this case, even if you use 1U, it is not really clear whether max_n_shift can be greater than 31. That said, I would not suggest to use ULL unless this is strictly necessary. Personally, I have no problem to submit another patch that adds U/UL suffixes to all shifted integer constants in the file :) but ... It seems that this driver has been ported from linux and this file still uses linux coding style, probably because deviations will reduce its maintainability. Adding a U suffix to those two might be considered an unjustified deviation. This can be solved by sending patch to Linux as well. This will also help Linux to reduce the number MISRA violations (I guess SMMUv3 will be part of the safety certification scope). At this stage the SMMU code is starting to deviate a lot from Linux so it will not be possible to update it easily to sync with latest linux code. And the decision should be are we fixing it or should we fully skip this file saying that it is coming from linux and should not be fixed. We started to have a discussion during the FuSa meeting on this but we need to come up with a conclusion per file. On this one I would say keeping it with linux code style and near from the linux one is not needed. I will address both point separately.In general, we don't want to mix coding style within a file. As the file started with the Linux one, then we should keep it like that. Otherwise, you will end up with a mix and it will be difficult for the contributor to know how to write new code. That said, I would not necessarily consider using "1 << ..." as part of the code style we want to keep. This brings to the second part (i.e. keeping the code near Linux). Linux has a much larger user/developper base than us. Therefore there are more chance for them to find bugs and fix them. By diverging, then we could end up to add new bugs and also increase the maintainance. I have tried both way with the SMMUv{1,2} driver. The first attempt was to fully adapt the code to Xen (coding style...). But this made difficult to keep track of bugs. A few months we removed it completely and then tried to keep as close as to Linux. Since then Linux reworked the IOMMU subsystem, so port needs to be adapted. It is more difficult but likely less than if we had our own port. So overall, I think it was beneficials for our version of the SMMU{v1, v2} drivers to be close to Linux. I haven't looked very close to the SMMUv3 to state whether we should stay close or not. Rahul, Julien, Stefano: what do you think here ? Rahul and you are the maintainers. I can share my preference/experience, but I think this is your call on how you want to maintain the driver. Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |