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
