On Thu, Jun 30, 2022 at 01:09:28PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 30, 2022 at 04:33:42PM +0530, Sebin Sebastian wrote:
> > On Thu, Jun 30, 2022 at 08:54:04AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 30, 2022 at 10:17:06AM +0530, Sebin Sebastian wrote:
> > > > Fix coverity warning dereferencing before null check. _ep and desc is
> > > > dereferenced on all paths until the check for null. Move the
> > > > initializations after the check for null.
> > >
> > > How can those values ever be NULL?
> > >
> > > > Coverity issue: 1518209
> > > >
> > > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > >
> > > kernel test robot did not find this issue.
> > >
> > After I submitted the PATCH v1, kernel test robot ran some tests and
> > produced a report of the things that I broke while creating the patch.
> > That's why I kept this tag.
>
> Yes, but the kernel test robot reported your first patch was broken, not
> that this commit itself was reported by that. Please drop that, it's
> confusing I know, and trips lots of people up, but is not needed here.
>
> >
> > > > Signed-off-by: Sebin Sebastian <mailmesebin00@xxxxxxxxx>
> > >
> > > What commit id does this change fix?
> > >
> > So should I provide the commit ID of the patch v1 that kernel
> > test robot referred to?
>
> No, report the commit id that this commit you are creating fixes. It
> had to be added to the tree sometime in the past, right?
>
> >
> > > > ---
> > > > Changes since v1: Fix the build errors and warnings due to first patch.
> > > > Fix the undeclared 'ep' and 'maxpacket' error. Fix the ISO C90 warning.
> > > >
> > > > drivers/usb/gadget/udc/aspeed_udc.c | 21 ++++++++++++++-------
> > > > 1 file changed, 14 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> > > > index d75a4e070bf7..a43cf8dde2a8 100644
> > > > --- a/drivers/usb/gadget/udc/aspeed_udc.c
> > > > +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> > > > @@ -341,26 +341,33 @@ static void ast_udc_stop_activity(struct ast_udc_dev *udc)
> > > > static int ast_udc_ep_enable(struct usb_ep *_ep,
> > > > const struct usb_endpoint_descriptor *desc)
> > > > {
> > > > - u16 maxpacket = usb_endpoint_maxp(desc);
> > > > - struct ast_udc_ep *ep = to_ast_ep(_ep);
> > >
> > > checking that ep is NULL here is an impossible thing on its own. You
> > > did change this so that you didn't check this anymore, which is odd as
> > > you did not mention that in the changelog text :(
> > >
> > Yes, I missed the checking for ep. I thought of checking it after
> > initilizing ep.
> >
> > > > - struct ast_udc_dev *udc = ep->udc;
> > > > - u8 epnum = usb_endpoint_num(desc);
> > > > unsigned long flags;
> > > > u32 ep_conf = 0;
> > > > u8 dir_in;
> > > > u8 type;
> > > > + u16 maxpacket;
> > > > + struct ast_udc_ep *ep;
> > > > + struct ast_udc_dev *udc;
> > > > + u8 epnum;
> > >
> > > Why did you reorder these?
> > >
> > This is actually the original order that these were in. I reordered it
> > while creating the first patch, then I changed it back to the original
> > order they were in the source tree for this patch.
>
> So this patch does not apply cleanly on linux-next? We did not apply
> your intermediate, broken, patch for obvious reasons, so you can not
> send a change on top of that, right?
>
> > > >
> > > > - if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
> > > > - maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> > > > + if (!_ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT) {
> > > > EP_DBG(ep, "Failed, invalid EP enable param\n");
> > > > return -EINVAL;
> > > > }
> > > > -
> > >
> > > Why did you remove this line?
> > >
> > I removed the check for maxpacket because it is not initialized in this
> > part, the check for the same thing comes after initialization.
> > This is the check for that, this is also included in the patch.
> > + if (maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> > + EP_DBG(ep, "Failed, invalid EP enable param\n");
> > + return -EINVAL;
> > + }
> > Should I add the check for 'ep' in this part?
> >
> > > Also, your To: line is messed up somehow, please fix your email
> > > client...
> > >
> > Ok, I will surely do it.
> >
> > > thanks,
> > >
> > > gre gk-h
> >
> > I did many mistakes in the patch v1, so I had to bring this patch to the
> > original state things were. I left all the declarations in the same
> > order (which made it seem like reordering) and moved the initialization
> > part after the check for _ep and desc.
>
>
> Perhaps you might want to start out doing coding style cleanups in
> drivers/staging/* to get the process of how to submit patches properly
> and test your changes before sending them out, before going out into the
> real part of the kernel.
>
> thanks,
>
> greg k-h
I am sorry to mess things up like this. I understand the entire process
of sending out patches and to not waste maintainer's time on these kind
of things.
This patch does apply cleanly on linux-next. There are no warnings or
errors while building and no checkpatch errors. Can I just send one
final patch including proper commit ID and the missing check for ep.
Thanks for pointing out all these mistakes.
