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]
Date: Fri, 5 Apr 2024 12:27:02 -0700
From: Bjorn Andersson <quic_bjorande@...cinc.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Johan Hovold
	<johan@...nel.org>,
        Krishna Kurapati <quic_kriskura@...cinc.com>,
        "Krzysztof
 Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring
	<robh@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
        Wesley Cheng
	<quic_wcheng@...cinc.com>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        "Conor
 Dooley" <conor+dt@...nel.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Felipe Balbi <balbi@...nel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_ppratap@...cinc.com>,
        <quic_jackp@...cinc.com>, Johan Hovold <johan+linaro@...nel.org>
Subject: Re: [PATCH v19 2/9] usb: dwc3: core: Access XHCI address space
 temporarily to read port info

On Fri, Apr 05, 2024 at 06:43:56AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 04, 2024 at 06:25:48PM -0700, Bjorn Andersson wrote:
> > On Thu, Apr 04, 2024 at 02:58:29PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> > > > On 04/04/2024 09:21, Johan Hovold wrote:
> > > > > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> > > > >  
> > > > >> +static int dwc3_get_num_ports(struct dwc3 *dwc)
> > > > >> +{
> > > > >> +	void __iomem *base;
> > > > >> +	u8 major_revision;
> > > > >> +	u32 offset;
> > > > >> +	u32 val;
> > > > >> +
> > > > >> +	/*
> > > > >> +	 * Remap xHCI address space to access XHCI ext cap regs since it is
> > > > >> +	 * needed to get information on number of ports present.
> > > > >> +	 */
> > > > >> +	base = ioremap(dwc->xhci_resources[0].start,
> > > > >> +		       resource_size(&dwc->xhci_resources[0]));
> > > > >> +	if (!base)
> > > > >> +		return PTR_ERR(base);
> > > > > 
> > > > > This is obviously still broken. You need to update the return value as
> > > > > well.
> > > > > 
> > > > > Fix in v20.
> > > > 
> > > > If one patchset reaches 20 versions, I think it is time to stop and
> > > > really think from the beginning, why issues keep appearing and reviewers
> > > > are still not happy.
> > > > 
> > > > Maybe you did not perform extensive internal review, which you are
> > > > encouraged to by your own internal policies, AFAIR. Before posting next
> > > > version, please really get some internal review first.
> > > 
> > > Also get those internal reviewers to sign-off on the commits and have
> > > that show up when you post them next.  That way they are also
> > > responsible for this patchset, it's not fair that they are making you do
> > > all the work here :)
> > > 
> > 
> > I like this idea and I'm open to us changing our way of handling this.
> > 
> > But unless such internal review brings significant input to the
> > development I'd say a s-o-b would take the credit from the actual
> > author.
> 
> It does not do that at all.  It provides proof that someone else has
> reviewed it and agrees with it.  Think of it as a "path of blame" for
> when things go bad (i.e. there is a bug in the submission.)  Putting
> your name on it makes you take responsibility if that happens.
> 

Right, this is why I like your idea.

But as s-o-b either builds a trail of who handled the patch, or reflects
that it was co-authored by multiple people, I don't think either one
properly reflects reality.

> > We've discussed a few times about carrying Reviewed-by et al from the
> > internal reviews, but as maintainer I dislike this because I'd have no
> > way to know if a r-b on vN means the patch was reviewed, or if it was
> > just "accidentally" carried from v(N-1).
> > But it might be worth this risk, is this something you think would be
> > appropriate?
> 
> For some companies we REQUIRE this to happen due to low-quality
> submissions and waste of reviewer's time.  Based on the track record
> here for some of these patchsets, hopefully it doesn't become a
> requirement for this company as well :)
> 

Interesting, I was under the impression that we (maintainers) didn't
want such internally originating tags.

If that's not the case, then I'd be happy to adjust our internal
guidelines.

Regards,
Bjorn

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ