Re: [PATCH 03/14] staging: clocking-wizard: Split probe function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 15, 2018 at 05:30:15AM +1000, James Kelly wrote:
> Dan,
> 
> > On 14 May 2018, at 11:47 pm, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > 
> > On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote:
> >> +static int clk_wzrd_probe(struct platform_device *pdev)
> >> +{
> >> +	int ret;
> >> +	struct device *dev = &pdev->dev;
> >> +
> >> +	ret = clk_wzrd_get_device_tree_data(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = clk_wzrd_regmap_alloc(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = clk_wzrd_register_ccf(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return 0;
> > 
> > The error handling is a terrible layer violation now...  Every
> > allocation function should have a matching free function.  But now
> > instead of that if clk_wzrd_register_ccf() fails then it starts cleaning
> > up the clk_wzrd->axi_clk that was allocated in
> > clk_wzrd_get_device_tree_data().
> > 
> > regards,
> > dan carpenter
> > 
> > 
> 
> This gets cleaned up in patch 6.  There seemed little point in putting a whole lot of
> code in patch 3 only to rip it out again in patch 6.  I was trying to keep the patches
> simple.

I later posted that it's actually buggy so it's not allow to introduce
a bug and then fix it.

I really quarrel with the word "simple".  As a reviewer it's my job to
spot bugs being introduced.  It took me a long time to review this patch
until I spotted the bug.  It's better to not take short cuts, but just
write error handling in the standard way.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux