linux-kernel - Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU mapping support

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: <s5hd43mnktvnuikuai25fhzchkt4y5w3ejabqhr43ihvwuefb3@wvatfe5k4loe>
Date: Fri, 30 Jan 2026 11:21:32 +0900
From: Koichiro Den <den@...inux.co.jp>
To: Niklas Cassel <cassel@...nel.org>
Cc: Aksh Garg <a-garg7@...com>, linux-pci@...r.kernel.org, 
	jingoohan1@...il.com, mani@...nel.org, lpieralisi@...nel.org, kwilczynski@...nel.org, 
	robh@...nel.org, bhelgaas@...gle.com, Zhiqiang.Hou@....com, 
	gustavo.pimentel@...opsys.com, linux-kernel@...r.kernel.org, s-vadapalli@...com, 
	danishanwar@...com
Subject: Re: [PATCH v4 2/3] PCI: dwc: ep: Add per-PF BAR and inbound ATU
 mapping support

On Thu, Jan 29, 2026 at 03:14:51PM +0100, Niklas Cassel wrote:
> On Thu, Jan 29, 2026 at 02:47:52PM +0530, Aksh Garg wrote:
> > -static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar)
> > +static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, u8 func_no, enum pci_barno bar)
> >  {
> > +	struct dw_pcie_ep_func *ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  	struct device *dev = pci->dev;
> >  	unsigned int i, num;
> > @@ -152,18 +157,18 @@ static void dw_pcie_ep_clear_ib_maps(struct dw_pcie_ep *ep, enum pci_barno bar)
> >  	u32 *indexes;
> 
> Hello Aksh,
> 
> Considering that all other functions that you have modified, you have added a:
> 
> if (!ep_func)
> 	return;
> 
> 
> I think you should do the same to this function.
> 
> 
> >  
> >  	/* Tear down the BAR Match Mode mapping, if any. */
> > -	if (ep->bar_to_atu[bar]) {
> > -		atu_index = ep->bar_to_atu[bar] - 1;
> > +	if (ep_func->bar_to_atu[bar]) {
> > +		atu_index = ep_func->bar_to_atu[bar] - 1;
> >  		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index);
> >  		clear_bit(atu_index, ep->ib_window_map);
> > -		ep->bar_to_atu[bar] = 0;
> > +		ep_func->bar_to_atu[bar] = 0;
> 
> Not related to your patch,
> 
> Koichiro (he is on To:),
> 
> don't you think that it would be clearer if we had a:
> 		return;
> 
> here...
> 
> I mean, a BAR can either have a BAR match mode mapping or a subrange mapping,
> but not both... So continuing executing code beyond this point seems pointless,
> possibly even confusing.

Thanks for pinging. I agree it would be clearer. Since it's orthogonal to
this series, would it be better if I sent a one-line patch?

By the way, Aksh, thank you for addressing the newly added address-match
mode case.

Koichiro

> 
> 
> >  	}
> >  
> >  	/* Tear down all Address Match Mode mappings, if any. */
> > -	indexes = ep->ib_atu_indexes[bar];
> > -	num = ep->num_ib_atu_indexes[bar];
> > -	ep->ib_atu_indexes[bar] = NULL;
> > -	ep->num_ib_atu_indexes[bar] = 0;
> > +	indexes = ep_func->ib_atu_indexes[bar];
> > +	num = ep_func->num_ib_atu_indexes[bar];
> > +	ep_func->ib_atu_indexes[bar] = NULL;
> > +	ep_func->num_ib_atu_indexes[bar] = 0;
> >  	if (!indexes)
> >  		return;
> 
> Sure, I see that this code will do a return here...
> 
> So there will be no harm done, but, having a simple return above
> would make it extra clear to the reader that is is always one or
> the other... you cannot have both.
> 
> If you agree, perhaps you could send a one liner patch?
> 
> 
> >  	for (i = 0; i < num; i++) {