On Tue, Jun 9, 2020 at 12:47 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote: > > > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv, > > > > sg_free_table(&acd->sgt); > > > > err_dma_map_sg: > > > > err_alloc_sg_table: > > > > > > So now we end up with two unnecessary labels. Probably best to delete two of these > > > three and name the remaining one appropriately: > > > > Hmm, I thought about it. But later decided to wait for review comments > > on the same in v1. > > I will remove it now. > > These are all unrelated to pin_user_pages(). Please don't do it in the > same patch. Staging code is there because it's ugly... If you don't > want to do unrelated changes to label names then you don't have to. What I am planning is to put this changes in a series. One patch will take care of pin_user_pages() related changes, 2nd patch will take care of minor bug fix in error path + level correction and 3rd patch will take care of set_page_dirty() -> set_page_dirty_lock(). Does it make sense ? > > Also on a personal note. The label name should say what the goto does > just like a function name says what the function does. "goto put_pages;" > Or "goto free_foo;". > > Don't do this: > > p = kmalloc(); > if (!p) > goto kmalloc_failed; > > This is a come-from label name and does not provide any information that > is not there on the line above... > > If you send a patch which uses your own personal style of label names, > I won't say anything about unless there is a bug. But just know in your > heart that you are wrong and I have silently reviewed your patch to > drivers/staging. > > regards, > dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
