linux-kernel - Re: [PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller

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: <757rsspeayo5yzfrlzd7pe7aqzkhiycjsow6rfgo75sqkaow5t@b3eahj6dclmb>
Date: Tue, 4 Mar 2025 08:41:44 +0800
From: Inochi Amaoto <inochiama@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>, 
	Inochi Amaoto <inochiama@...il.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Chen Wang <unicorn_wang@...look.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	sophgo@...ts.linux.dev, Yixun Lan <dlan@...too.org>, Longbin Li <looong.bin@...il.com>
Subject: Re: [PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI
 interrupt controller

On Mon, Mar 03, 2025 at 08:31:32PM +0100, Thomas Gleixner wrote:
> On Mon, Mar 03 2025 at 19:16, Inochi Amaoto wrote:
> > Add support for Sophgo SG2044 MSI interrupt controller.
> 
> This patch fails to apply on top of:
> 
>      git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/drivers
> 
> Please always ensure that your patches apply against the tree/branch
> into which they are supposed to be merged. Grabbing random patches from
> the mailing list as base is not sufficient. It's clearly documented
> against what you should work.
>      

Thanks for pointing that, I will check the tip tree and see what
is conflicted. I forgot there will be something changed when
merging patch.

> > +struct sg2042_msi_of_data {
> 
> There is nothing specific to OF in this data structure. This structure
> contains the chip and the MSI parent ops of each variant. So something
> like sg204x_chip_info is way more descriptive.
> 

Yeah, chip_info it more clear than of_data. I have forgotten this
driver is not just for dtb but also UEFI fdt.

> > +	const struct irq_chip		*irqchip;
> > +	const struct msi_parent_ops	*parent_ops;
> > +};
> > +
> >  struct sg2042_msi_chipdata {
> 
> and rename that one to sg204x_... as it is not longer sg2042 specific.
> 

This is OK for me.

> >  	void __iomem	*reg_clr;	// clear reg, see TRM, 10.1.33, GP_INTR0_CLR
> >  
> > @@ -29,8 +34,10 @@ struct sg2042_msi_chipdata {
> >  	u32		irq_first;	// The vector number that MSIs starts
> >  	u32		num_irqs;	// The number of vectors for MSIs
> >  
> > -	DECLARE_BITMAP(msi_map, SG2042_MAX_MSI_VECTOR);
> > +	unsigned long	*msi_map;
> >  	struct mutex	msi_map_lock;	// lock for msi_map
> > +
> > +	const struct sg2042_msi_of_data	*data;
> 
> Please keep the tabular formatting of this struct. See:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes
> 
> >  };
> >  
> >  static int sg2042_msi_allocate_hwirq(struct sg2042_msi_chipdata *data, int num_req)
> > @@ -81,6 +88,37 @@ static const struct irq_chip sg2042_msi_middle_irq_chip = {
> >  	.irq_compose_msi_msg	= sg2042_msi_irq_compose_msi_msg,
> >  };
> >  
> > +static void sg2044_msi_irq_ack(struct irq_data *d)
> > +{
> > +	struct sg2042_msi_chipdata *data = irq_data_get_irq_chip_data(d);
> > +
> > +	writel(0, (unsigned int *)data->reg_clr + d->hwirq);
> > +
> 
> Pointless newline
> 
> > +	irq_chip_ack_parent(d);
> > +}
> > +
> > +static void sg2044_msi_irq_compose_msi_msg(struct irq_data *d,
> > +					   struct msi_msg *msg)
> 
> No line break required. Please use up to 100 characters.
> 
> >  static int sg2042_msi_parent_domain_alloc(struct irq_domain *domain,
> >  					  unsigned int virq, int hwirq)
> >  {
> > @@ -119,7 +157,7 @@ static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
> >  			goto err_hwirq;
> >  
> >  		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > -					      &sg2042_msi_middle_irq_chip, data);
> > +					      data->data->irqchip, data);
> 
> The conversion of the existing code to this should be a preparatory patch
> for ease of review and the support for the new chip built on top.
> 
> Also please come up with a sensible name for this new 'data' pointer.
> 
>      data->data->
> 
> is horribly unintuitive. It's not the same data type. 
> 
>      data->chip_info
> 
> or such makes it clear what this is about.
> 

Good, I will take care of that.

Regards,
Inochi