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

[Serge Semin]

On Thu, May 28, 2020 at 04:51:03PM +0300, Andy Shevchenko wrote:
> On Thu, May 28, 2020 at 4:40 PM Serge Semin
> <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, May 28, 2020 at 01:00:57AM +0300, Andy Shevchenko wrote:
> > > On Tuesday, May 26, 2020, Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxxxxxxxx>
> > > > Cc: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > > > Cc: Paul Burton <paulburton@xxxxxxxxxx>
> > > > Cc: Olof Johansson <olof@xxxxxxxxx>
> > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > > Cc: linux-mips@xxxxxxxxxxxxxxx
> > > > Cc: soc@xxxxxxxxxx
> > > > Cc: devicetree@xxxxxxxxxxxxxxx
> > > >
> >
> > [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.
> 

> __device_release_driver() calls dev_set_drvdata(dev, NULL);
> What did I miss?
> 
> > 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.
> 
> The above is right and I agree with.

Hm, good point. I missed that part of __device_release_driver(). So do you
suggest to remove that cleanup?

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko