netdev - RE: [RFC PATCH net-next v5 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <015301d978d5$5e74f270$1b5ed750$@trustnetic.com>
Date:   Thu, 27 Apr 2023 14:56:19 +0800
From:   Jiawen Wu <jiawenwu@...stnetic.com>
To:     "'Andy Shevchenko'" <andy.shevchenko@...il.com>
Cc:     <netdev@...r.kernel.org>, <jarkko.nikula@...ux.intel.com>,
        <andriy.shevchenko@...ux.intel.com>,
        <mika.westerberg@...ux.intel.com>, <jsd@...ihalf.com>,
        <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
        <linux-i2c@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <mengyuanlou@...-swift.com>
Subject: RE: [RFC PATCH net-next v5 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

On Thursday, April 27, 2023 1:57 PM, Andy Shevchenko wrote:
> On Thu, Apr 27, 2023 at 5:15 AM Jiawen Wu <jiawenwu@...stnetic.com> wrote:
> > On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@...il.com wrote:
> > > Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti:
> 
> ...
> 
> > > > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
> > > > +{
> > > > +   struct platform_device *pdev = to_platform_device(dev->dev);
> > > > +   struct resource *r;
> > > > +
> > > > +   r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +   if (!r)
> > > > +           return -ENODEV;
> > > > +
> > > > +   dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> > > > +
> > > > +   return PTR_ERR_OR_ZERO(dev->base);
> > > > +}
> > >
> > > Redundant. See below.
> 
> ...
> 
> > > >     case MODEL_BAIKAL_BT1:
> > > >             ret = bt1_i2c_request_regs(dev);
> > > >             break;
> > > > +   case MODEL_WANGXUN_SP:
> > > > +           ret = txgbe_i2c_request_regs(dev);
> > >
> > > How is it different to...
> > >
> > > > +           break;
> > > >     default:
> > > >             dev->base = devm_platform_ioremap_resource(pdev, 0);
> > >
> > > ...this one?
> >
> > devm_platform_ioremap_resource() has one more devm_request_mem_region()
> > operation than devm_ioremap(). By my test, this memory cannot be re-requested,
> > only re-mapped.
> 
> Yeah, which makes a point that the mother driver requests a region
> that doesn't belong to it. You need to split that properly in the
> mother driver and avoid requesting it there. Is it feasible? If not,
> why?

The I2C region belongs to the middle part of the total region. It was not considered to
split because the mother driver implement I2C bus master driver itself in the previous
patch. But is it suitable for splitting? After splitting, I get two virtual address, and each
time I read/write to a register, I have to determine which region it belongs to...Right?

> 
> ...
> 
> > > >     dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > >
> > > > +   if (!dev->flags)
> > >
> > > No need to check this. Just define priorities (I would go with above to be
> > > higher priority).
> > >
> > > > +           device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags);
> > >
> > > Needs to be added to the Device Tree bindings I believe.
> > >
> > > But wait, don't we have other ways to detect your hardware at probe time and
> > > initialize flags respectively?
> >
> > I2C is connected to our NIC chip with no PCI ID, so I register a platform device for it.
> > Please see the 4/9 patch. Software nodes are used to pass the device structure but
> > no DT and ACPI. I haven't found another way to initialize flags yet, other than the
> > platform data used in the previous patch (it seems to be an obsolete way).
> 
> You can share a common data structure between the mother driver and
> her children. In that case you may access it via
> `dev_get_drvdata(pdev.dev->parent)` call.
> 

I'd like to try it.

> OTOH, the property, if only Linux (kernel) specific for internal
> usage, should be named accordingly, or be prepared to have one in
> Device Tree / ACPI / etc. Examples: USB dwc3 driver (see "linux,"
> ones), or intel-lpss-pci.c/intel-lpss-acpi.c (see the SPI type).
> 
> --
> With Best Regards,
> Andy Shevchenko
>