Re: [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd

[Jules Maselbas <jmaselbas@kalray.eu>]

Hi Sascha,

On Thu, Mar 26, 2020 at 07:03:11AM +0100, Sascha Hauer wrote:
> Hi Jules,
> 
> >  
> > @@ -132,6 +134,10 @@ struct file_list_entry *dfu_file_entry;
> >  static int dfufd = -EINVAL;
> >  static struct file_list *dfu_files;
> >  static int dfudetach;
> > +static struct mtd_info_user dfu_mtdinfo;
> > +static loff_t dfu_written;
> > +static loff_t dfu_erased;
> > +static int prog_erase;
> 
> These are not initialized, this works for the first file updated via
> DFU, but what about the following ones?
Good point, this initialization is be required between to file.
I don't think cleaning this in unbind will be sufficient, I will
try to update multiple files.

> 
> >  
> >  /* USB DFU functional descriptor */
> >  static struct usb_dfu_func_descriptor usb_dfu_func = {
> > @@ -327,8 +333,16 @@ static void dfu_cleanup(struct f_dfu *dfu)
> >  static void dn_complete(struct usb_ep *ep, struct usb_request *req)
> >  {
> >  	struct f_dfu		*dfu = req->context;
> > +	loff_t size;
> >  	int ret;
> >  
> > +	if (prog_erase && (dfu_written + req->length) > dfu_erased) {
> > +		size = roundup(req->length, dfu_mtdinfo.erasesize);
> > +		erase(dfufd, size, dfu_erased);
> 
> You should check the return value here.
Right
 
> > +		dfu_erased += size;
> > +	}
> > +
> > +	dfu_written += req->length;
> >  	ret = write(dfufd, req->buf, req->length);
> >  	if (ret < (int)req->length) {
> >  		perror("write");
> > @@ -493,7 +507,12 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> >  			}
> >  
> >  			if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) {
> > -				ret = erase(dfufd, ERASE_SIZE_ALL, 0);
> > +				ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo);
> > +				if (ret) /* not a mtd */
> > +					ret = erase(dfufd, ERASE_SIZE_ALL, 0);
> > +				else
> > +					prog_erase = 1;
> 
> I am not aware of any non mtd devices that need erase. I think you can
> drop the full erase here.
OK

Thanks for the review,
Jules

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox