Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for ch9 udc

[gre...@linuxfoundation.org]

On Thu, Jun 29, 2023 at 05:56:30AM +, Christophe Leroy wrote:
> 
> 
> Le 28/06/2023 à 23:10, Leo Li a écrit :
> > 
> > 
> >> -Original Message-
> >> From: Christophe Leroy 
> >> Sent: Wednesday, June 28, 2023 2:40 PM
> >> To: Leo Li ; Ma Ke 
> >> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; linuxppc-
> >> d...@lists.ozlabs.org; linux-ker...@vger.kernel.org
> >> Subject: Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for
> >> ch9 udc
> >>
> >>
> >>
> >> Le 28/06/2023 à 19:04, Leo Li a écrit :
> >>>
> >>>
> >>>> -Original Message-
> >>>> From: Ma Ke 
> >>>> Sent: Wednesday, June 28, 2023 3:15 AM
> >>>> To: Leo Li 
> >>>> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; linuxppc-
> >>>> d...@lists.ozlabs.org; linux-ker...@vger.kernel.org; Ma Ke
> >>>> 
> >>>> Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for
> >>>> ch9 udc
> >>>>
> >>>> We should verify the bound of the array to assure that host may not
> >>>> manipulate the index to point past endpoint array.
> >>>>
> >>>> Signed-off-by: Ma Ke 
> >>>> ---
> >>>>drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++
> >>>>1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c
> >>>> b/drivers/usb/gadget/udc/fsl_qe_udc.c
> >>>> index 3b1cc8fa30c8..f4e5cbd193b7 100644
> >>>> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c
> >>>> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
> >>>> @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8
> >>>> request_type, u16 value,
> >>>>  } else if ((request_type & USB_RECIP_MASK) ==
> >>>> USB_RECIP_ENDPOINT) {
> >>>>  /* Get endpoint status */
> >>>>  int pipe = index & USB_ENDPOINT_NUMBER_MASK;
> >>>> +if (pipe >= USB_MAX_ENDPOINTS)
> >>>> +goto stall;
> >>>
> >>> Thanks.  This seems to be the right thing to do.  But normally we don't 
> >>> mix
> >> declarations with code within a code block.  Could we re-arrange the code a
> >> little bit so declarations stay on top?
> >>
> >> But we are at the start of a code block aren't we ?
> > 
> > But they were at the beginning of a { } block which is compliant with the 
> > C89 standard.  I know gcc is more relaxed from this.  But it is probably 
> > still good to stick to the standard?
> 
> Sorry I misread the patch and failed to see that the declaration block 
> was continuing after the change.
> 
> So yes don't interleave code with declarations. Leave declaration at the 
> top of a block with a blank line between declarations and code.

This is fine as-is, no need to change anything.

greg k-h

Re: bug: usb: gadget: FSL_UDC_CORE Corrupted request list leads to unrecoverable loop.

[gre...@linuxfoundation.org]

On Fri, Feb 18, 2022 at 11:17:59AM +, Joakim Tjernlund wrote:
> I was happy with commit msgs and I don't know what the criticism was.

I have no context anymore, sorry.

Can someone resubmit the change again and we can take it from there?

thanks,

greg k-h

Re: bug: usb: gadget: FSL_UDC_CORE Corrupted request list leads to unrecoverable loop.

[gre...@linuxfoundation.org]

On Fri, Feb 18, 2022 at 10:21:12AM +, Joakim Tjernlund wrote:
> I think you could apply them as is, only criticism was the commit msgs.

That is always a good reason to reject a change.  Please resubmit them
with the commit message cleaned up and I will be glad to review it
again.

thanks,

greg k-h

Re: [PATCH] [Rebased for 4.19 and 4.14] powerpc/32: Fix boot failure with GCC latent entropy plugin

[gre...@linuxfoundation.org]

On Sun, Jan 30, 2022 at 09:45:10AM +, Christophe Leroy wrote:
> This is backport for 4.19 and 4.14
> 
> (cherry picked from commit bba496656a73fc1d1330b49c7f82843836e9feb1)
> 
> Boot fails with GCC latent entropy plugin enabled.

All now queued up, thanks,

greg k-h

Re: [PATCH] [Modified for 5.16 and 5.15] powerpc/32s: Fix kasan_init_region() for KASAN

[gre...@linuxfoundation.org]

On Sat, Jan 29, 2022 at 05:26:10PM +, Christophe Leroy wrote:
> This is a backport for 5.16 and 5.15.
> 
> To apply, it also requires commit 37eb7ca91b69 ("powerpc/32s: Allocate
> one 256k IBAT instead of two consecutives 128k IBATs")

Thanks for these, now queued up.

greg k-h

Re: [PATCH 1/3] powerpc/powernv: remove the unused pnv_pci_set_p2p function

['gre...@linuxfoundation.org']

On Tue, Jul 09, 2019 at 06:06:54PM +0300, Max Gurtovoy wrote:
> 
> On 7/9/2019 5:40 PM, Christoph Hellwig wrote:
> > On Tue, Jul 09, 2019 at 05:37:18PM +0300, Max Gurtovoy wrote:
> > > On 7/9/2019 5:32 PM, Christoph Hellwig wrote:
> > > > On Tue, Jul 09, 2019 at 05:31:37PM +0300, Max Gurtovoy wrote:
> > > > > Are we ok with working on a solution during kernel-5.3 cycle ?
> > > > You can start working on it any time, no need to ask for permission.
> > > I just want to make sure we don't remove it from the kernel before we send
> > > a general API solution.
> > The code is gone in this merge window.
> 
> Ok, so we must fix it to kernel-5.3 to make sure we're covered.
> 
> Understood.
> 
> > 
> > > This way we'll make sure that all the kernel versions has this
> > > functionality...
> > Again, we do not provide functionality for out of tree modules.  We've
> > had the p2p API for about a year now, its not like you didn't have
> > plenty of time.
> 
> I didn't know about the intention to remove this code...

The original email you responded to in this thread was received by you
back in May.  It is now July, 5.3 will not be out for 8-9 weeks.  There
has been plenty of time here...

greg k-h

Re: [PATCH 5/5] Lib: sort.h: replace int size with size_t size in the swap function

[gre...@linuxfoundation.org]

On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote:
> Replace int type with size_t type of the size argument
> in the swap function, also affect all its dependencies.

This says _what_ the patch does, but it gives no clue as to _why_ you
are doing this.  Neither did your 0/5 patch :(

Why make this change?  Nothing afterward depends on it from what I can
tell, so why is it needed?

thanks,

greg k-h

Re: SV: MPC8321 boot failure

[gre...@linuxfoundation.org]

On Thu, Oct 18, 2018 at 08:51:46AM +, David Gounaris wrote:
> Hi, I can also confirm that it works after cherry-picking the proposed commit.
> 
> Reported-and-tested-by: David Gounaris 
> mailto:david.gouna...@infinera.com>>
> 

Now queued up, thanks.

greg k-h

Re: [PATCH 2/9] Move dma_ops from archdata into struct device

[gre...@linuxfoundation.org]

On Wed, Jan 11, 2017 at 10:28:05PM +, Bart Van Assche wrote:
> On Wed, 2017-01-11 at 21:31 +0100, gre...@linuxfoundation.org wrote:
> > That's a big sign that your patch series needs work.  Break it up into
> > smaller pieces, it should be possible, which will make merges easier
> > (well, different in a way.)
> 
> Hello Greg,
> 
> Can you have a look at the attached patches? These three patches are a
> splitup of the single patch at the start of this e-mail thread.

Please send them in the proper format (i.e. one patch per email), and I
will be glad to review them.  Otherwise it's really hard to do so, would
you want to review attachments?

thanks,

greg k-h

Re: [PATCH 2/9] Move dma_ops from archdata into struct device

[gre...@linuxfoundation.org]

On Wed, Jan 11, 2017 at 06:17:03PM +, Bart Van Assche wrote:
> On Wed, 2017-01-11 at 07:48 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 10, 2017 at 04:56:41PM -0800, Bart Van Assche wrote:
> > > Several RDMA drivers, e.g. drivers/infiniband/hw/qib, use the CPU to
> > > transfer data between memory and PCIe adapter. Because of performance
> > > reasons it is important that the CPU cache is not flushed when such
> > > drivers transfer data. Make this possible by allowing these drivers to
> > > override the dma_map_ops pointer. Additionally, introduce the function
> > > set_dma_ops() that will be used by a later patch in this series.
> > 
> > When you say things like "additionally", that's a huge flag that this
> > needs to be split up into multiple patches.  No need to add
> > set_dma_ops() here in this patch.
> 
> Hello Greg,
> 
> Some architectures already define a set_dma_ops() function. So what this
> patch does is to move both the dma_ops pointer and the set_dma_ops()
> function from architecture-specific to architecture independent code. I
> don't think that it is possible to separate these two changes. But I
> understand that how I formulated the patch description caused confusion. I
> will rewrite the patch description to make it more clear before I repost
> this patch series.

I think you should separate it out into multiple patches, this is a
mess, as you say below:

> > And I'd argue that it should be dma_ops_set(), and dma_ops_get(), just
> > to keep the namespace sane, but that's probably a different set of
> > patches...
> 
> Every time I rebase and retest this patch series on top of a new kernel
> version I have to modify some of the patches to compensate for changes in
> the architecture code. So I expect that once Linus merges these patches that
> he will have to resolve one or more merge conflicts. Including a rename of
> the functions that query and set the dma_ops pointer in this patch series
> would increase the number of merge conflicts triggered by this patch series
> and would make Linus' job harder. So I hope that you will allow me to
> postpone that rename until a later time ...

That's a big sign that your patch series needs work.  Break it up into
smaller pieces, it should be possible, which will make merges easier
(well, different in a way.)

Good luck, tree-wide changes are not simple.

greg k-h

Re: [PATCH 2/9] Move dma_ops from archdata into struct device

[gre...@linuxfoundation.org]

On Wed, Jan 11, 2017 at 06:03:15PM +, Bart Van Assche wrote:
> On Wed, 2017-01-11 at 07:46 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 10, 2017 at 04:56:41PM -0800, Bart Van Assche wrote:
> > > Several RDMA drivers, e.g. drivers/infiniband/hw/qib, use the CPU to
> > > transfer data between memory and PCIe adapter. Because of performance
> > > reasons it is important that the CPU cache is not flushed when such
> > > drivers transfer data. Make this possible by allowing these drivers to
> > > override the dma_map_ops pointer. Additionally, introduce the function
> > > set_dma_ops() that will be used by a later patch in this series.
> > > 
> > > Signed-off-by: Bart Van Assche 
> > > Cc: [ ... ]
> > 
> > That's a crazy cc: list, you should break this up into smaller pieces,
> > otherwise it's going to bounce...
> 
> That's a subset of what scripts/get_maintainer.pl came up with. Suggestions
> for a more appropriate cc-list for a patch like this that touches all
> architectures would be welcome.

You need to break this patch up into a series that can be applied in
sequence, don't change everything all at once.  That's a mess to merge,
as you are finding out.

> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 491b4c0ca633..c7cb225d36b0 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -885,6 +885,8 @@ struct dev_links_info {
> > >   * a higher-level representation of the device.
> > >   */
> > >  struct device {
> > > + const struct dma_map_ops *dma_ops; /* See also get_dma_ops() */
> > > +
> > >   struct device   *parent;
> > >  
> > >   struct device_private   *p;
> > 
> > Why not put this new pointer down with the other dma fields in this
> > structure?  Any specific reason it needs to be first?
> 
> Are there CPU architectures for which access to the first member of a
> structure can be encoded and/or executed more efficiently than access to
> other members of a structure? If not, I'm fine with moving the new pointer
> further down.

Why do you think that your pointer is the one that gets to be "most
efficient"?  :)

Seriously, no, it doesn't matter at all, it's all just pointer math
which is very fast.  Put it with the other stuff please, don't try to
optimize something without ever measuring it.

thanks,

greg k-h

Re: [v12, 0/8] Fix eSDHC host version register bug

[gre...@linuxfoundation.org]

On Wed, Oct 19, 2016 at 02:47:07AM +, Y.B. Lu wrote:
> + Greg
> 
> Hi Greg,
> 
> I submitted this patchset for a MMC bug fix, and introduce the below patch 
> which needs your ACK.
> > > Arnd Bergmann (1):
> > >   base: soc: introduce soc_device_match() interface
> https://patchwork.kernel.org/patch/9342913/
> 
> Could you help to review it and give some comments or ACK.
> Thank you very much.

Now acked.

Re: warnings from drivers/tty/ehv_bytechan.c

[gre...@linuxfoundation.org]

On Mon, Feb 20, 2012 at 01:24:22PM +, Tabi Timur-B04825 wrote:
 Stephen Rothwell wrote:
  console_initcall() is not defined for modules.
 
 Hmmm... the patch you posted is a good short-term fix, but I wonder if 
 makes sense for the driver to support modules at all.  I have this in the 
 driver:
 
 #include linux/module.h
 ...
 module_init(ehv_bc_init);
 module_exit(ehv_bc_exit);
 
 although to be honest, I can't remember the last time I tried to compile 
 it as a module.
 
 The problem stems from the fact that it's a console driver *and* a tty 
 driver.  It makes sense that a tty driver can be compiled as a module, but 
 not a console driver.
 
 So Greg, can I do something like this:
 
 #ifdef MODULE
 module_initcall(ehv_bc_console_init)
 #else
 console_initcall(ehv_bc_console_init);
 #endif

Sure, something like that is fine, but if the code really can't be a
module, why not just fix the Kconfig file to enforce this properly
instead?

thanks,

greg k-h
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: warnings from drivers/tty/ehv_bytechan.c

[gre...@linuxfoundation.org]

On Fri, Feb 24, 2012 at 04:00:12PM -0600, Timur Tabi wrote:
 gre...@linuxfoundation.org wrote:
  Sure, something like that is fine, but if the code really can't be a
  module, why not just fix the Kconfig file to enforce this properly
  instead?
 
 That's the simplest approach, for use.  The TTY portion of the driver can
 be used as a module.  Is there any real value in loading a TTY driver as a
 module?

Depends on the hardware it supports :)

 In this case, the console support for byte channels would not be
 available.

Then it doesn't make sense, right?

thanks,

greg k-h

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: warnings from drivers/tty/ehv_bytechan.c

[gre...@linuxfoundation.org]

On Fri, Feb 24, 2012 at 04:15:04PM -0600, Timur Tabi wrote:
 gre...@linuxfoundation.org wrote:
   That's the simplest approach, for use.  The TTY portion of the driver can
   be used as a module.  Is there any real value in loading a TTY driver as 
   a
   module?
 
  Depends on the hardware it supports :)
  
   In this case, the console support for byte channels would not be
   available.
 
  Then it doesn't make sense, right?
 
 I guess that's my question.  Is there a real use case for having console
 output go to the serial port, and TTY go to a byte channel?  Even if you
 wanted to do that, I supposed you don't need to load the byte channel
 driver as a module to get that behavior.
 
 Anyway, that's all academic.  A more important question is: now that the
 driver can't be compiled as a module, should I change module_init() to
 something else (like device_initcall)?
 
 Should I remove this line?
 
   #include linux/module.h

No, no need to, leave it as-is if it builds properly.

greg k-h
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev