On Tue, Jun 15, 2021 at 12:51:38PM +0100, Robin Murphy wrote:
> On 2021-06-15 12:34, Will Deacon wrote:
> > On Tue, Jun 15, 2021 at 07:22:10PM +0800, Leizhen (ThunderTown) wrote:
> > >
> > >
> > > On 2021/6/11 18:32, Will Deacon wrote:
> > > > On Wed, Jun 09, 2021 at 08:54:38PM +0800, Zhen Lei wrote:
> > > > > Fixes scripts/checkpatch.pl warning:
> > > > > WARNING: Possible unnecessary 'out of memory' message
> > > > >
> > > > > Remove it can help us save a bit of memory.
> > > > >
> > > > > Signed-off-by: Zhen Lei <thunder.leiz...@huawei.com>
> > > > > ---
> > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++------
> > > > > 1 file changed, 2 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > > index 2ddc3cd5a7d1..fd7c55b44881 100644
> > > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > > @@ -2787,10 +2787,8 @@ static int arm_smmu_init_l1_strtab(struct
> > > > > arm_smmu_device *smmu)
> > > > > void *strtab = smmu->strtab_cfg.strtab;
> > > > > cfg->l1_desc = devm_kzalloc(smmu->dev, size, GFP_KERNEL);
> > > > > - if (!cfg->l1_desc) {
> > > > > - dev_err(smmu->dev, "failed to allocate l1 stream table
> > > > > desc\n");
> > > > > + if (!cfg->l1_desc)
> > > >
> > > > What error do you get if devm_kzalloc() fails? I'd like to make sure
> > > > it's
> > > > easy to track down _which_ allocation failed in that case -- does it
> > > > give
> > > > you a line number, for example?
> > >
> > > When devm_kzalloc() fails, the OOM information is printed. No line number
> > > information, but the
> > > size(order) and call stack is printed. It doesn't matter which allocation
> > > failed, the failure
> > > is caused by insufficient system memory rather than the fault of the SMMU
> > > driver. Therefore,
> > > the current printing is not helpful for locating the problem of
> > > insufficient memory. After all,
> > > when memory allocation fails, the SMMU driver cannot work at a lower
> > > specification.
> >
> > I don't entirely agree. Another reason for the failure is because the driver
> > might be asking for a huge (or negative) allocation, in which case it might
> > be instructive to have a look at the actual caller, particularly if the
> > size is derived from hardware or firmware properties.
>
> Agreed - other than deliberately-contrived situations I don't think I've
> ever hit a genuine OOM, but I definitely have debugged attempts to allocate
> -1 of something. If the driver-specific message actually calls out the
> critical information, e.g. "failed to allocate %d stream table entries", it
> gives debugging a head start since the miscalculation is obvious, but a
> static message that only identifies the callsite really only saves a quick
> trip to scripts/faddr2line, and personally I've never found that
> particularly valuable.
So it sounds like this particular patch is fine, but the one for smmuv2
should leave the IRQ allocation message alone (by virtue of it printing
something a bit more useful -- the number of irqs).
Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
