Re: [PATCH v3 4/6] bus: Add Baikal-T1 AXI-bus driver

[Serge Semin <Sergey.Semin@baikalelectronics.ru>]

On Thu, May 28, 2020 at 01:00:57AM +0300, Andy Shevchenko wrote:
> On Tuesday, May 26, 2020, Serge Semin <Sergey.Semin@baikalelectronics.ru>
> wrote:
> 
> > AXI3-bus is the main communication bus connecting all high-speed
> > peripheral IP-cores with RAM controller and MIPS P5600 cores on Baikal-T1
> > SoC. Bus traffic arbitration is done by means of DW AMBA 3 AXI
> > Interconnect (so called AXI Main Interconnect) routing IO requests from
> > one SoC block to another. This driver provides a way to detect any bus
> > protocol errors and device not responding situations by means of an
> > embedded on top of the interconnect errors handler block (EHB). AXI
> > Interconnect QoS arbitration tuning is currently unsupported.
> > The bus doesn't provide a way to detect the interconnected devices,
> > so they are supposed to be statically defined like by means of the
> > simple-bus sub-nodes.
> 
> 
> 
> Few comments in case if you need a new version. Main point is about
> sysfs_streq().

Hello, Andy. Thanks for your comments. I'll address most of them in a follow-up
patches. See just one note below.

> 
> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Olof Johansson <olof@lixom.net>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: linux-mips@vger.kernel.org
> > Cc: soc@kernel.org
> > Cc: devicetree@vger.kernel.org
> >

[nip]

> > +
> > +static void bt1_axi_clear_data(void *data)
> > +{
> > +       struct bt1_axi *axi = data;
> > +       struct platform_device *pdev = to_platform_device(axi->dev);
> > +
> > +       platform_set_drvdata(pdev, NULL);
> 
> 

> Doesn't device driver core do this already?

It doesn't on remove. This cleanups the drvdata pointer when the driver is
unloaded at the moment of remove() callback calling. This is a good
practice to leave the device the same as it has been before usage including
the pointer value. In this case if theoretically someone (though very
unlikely, but anyway) would try to use the pointer without having it
initialized, the NULL-dereference would pop up, otherwise we may
corrupt someones memory, which is very nasty.

-Sergey