linux-kernel - Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240620191923.3d62c128@bootlin.com>
Date: Thu, 20 Jun 2024 19:19:23 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>, Simon Horman
 <horms@...nel.org>, Sai Krishna Gajula <saikrishnag@...vell.com>, Thomas
 Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Lee Jones
 <lee@...nel.org>, Arnd Bergmann <arnd@...db.de>, Horatiu Vultur
 <horatiu.vultur@...rochip.com>, UNGLinuxDriver@...rochip.com, Andrew Lunn
 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, Saravana Kannan <saravanak@...gle.com>, Bjorn
 Helgaas <bhelgaas@...gle.com>, Philipp Zabel <p.zabel@...gutronix.de>, Lars
 Povlsen <lars.povlsen@...rochip.com>, Steen Hegelund
 <Steen.Hegelund@...rochip.com>, Daniel Machon
 <daniel.machon@...rochip.com>, Alexandre Belloni
 <alexandre.belloni@...tlin.com>, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, netdev@...r.kernel.org,
 linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, Allan
 Nielsen <allan.nielsen@...rochip.com>, Luca Ceresoli
 <luca.ceresoli@...tlin.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

Hi Andy,

On Thu, 20 Jun 2024 18:43:09 +0200
Herve Codina <herve.codina@...tlin.com> wrote:

> My bad, I wrongly answered first in private.
> I already eesend my answers with people in Cc
> 
> Now, this is the Andy's your reply.
> 
> Sorry for this mistake.
> 
> Herve
> 
> On Thu, 20 Jun 2024 18:07:16 +0200
> Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> 
> > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@...tlin.com> wrote:  
> > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > Andy Shevchenko <andy.shevchenko@...il.com> wrote:    
> > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:    
> > 
> > ...
> >   
> > > > > +   if (!dev->of_node) {
> > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > +           return -EINVAL;
> > > > > +   }    
> > > >
> > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > already for this case.    
> > >
> > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > >
> > > The check here is to ensure that an of_node is available as it will be use
> > > for DT overlay loading.    
> > 
> > So, what exactly do you want to check? fwnode check covers this.
> >   
> > > I will keep the check here and use dev_of_node() instead of dev->of_node.    
> > 
> > It needs to be well justified as from a coding point of view this is a
> > duplication.

On DT based system, if a fwnode is set it is an of_node.
On ACPI, if a fwnode is set is is an acpi_node.

The core PCI, when it successfully creates the DT node for a device
(CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
So we can have a device with:
 - fwnode from ACPI
 - of_node from core PCI creation

This driver needs the of_node to load the overlay.
Even if the core PCI cannot create a DT node for the PCI device right
now, I don't expect this LAN855x PCI driver updated when the core PCI
is able to create this PCI device DT node.

> > 
> > ...
> >   
> > > > > +   pci_set_master(pdev);    
> > > >
> > > > You don't use MSI, what is this for?    
> > >
> > > DMA related.
> > > Allows the PCI device to be master on the bus and so initiate transactions.
> > >
> > > Did I misunderstood ?    
> > 
> > So, you mean that the PCI device may initiate DMA transactions and
> > they are not related to MSI, correct?

That's my understanding.
Right now, the internal LAN966x DMA controller is not used but it will be
used in a near future.

> > 
> > ...
> >   
> > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > +   { PCI_DEVICE(0x1055, 0x9660) },    
> > > >
> > > > Don't you have VENDOR_ID defined somewhere?    
> > >
> > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > >
> > > I will patch pci-ids.h to create:
> > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > >
> > > And use PCI_VENDOR_ID_MCHP in this series.    
> > 
> > Okay, but I don't think (but I haven't checked) we have something like
> > this ever done there. In any case it's up to Bjorn how to implement
> > this.

Right, I wait for Bjorn reply before changing anything.

Best regards,
Hervé