| Message ID | 0da6859b-40cc-4b3e-b8b6-fed157517083@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: loongson: Add a check for lsdc_bo_create() errors | expand |
Hi, Thanks for the patch. The commit title generally should be 'drm/looongson: Add a check for lsdc_bo_create() errors' not 'drm: loongson: xxxx' On 2023/7/18 15:01, Dan Carpenter wrote: > The lsdc_bo_create() function can fail so add a check for that. > > Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller") Please drop this Fixes tag, because you patch just add a error handling. Yes, the lsdc_bo_create() function can fail, but this is generally not happen. Even if the fail happened, your patch is not fixing the root problem. I know that you create this patch with the bare brain, I would like hear more practical usage or bugs of this driver. And mention more in the commit message is preferred. > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c b/drivers/gpu/drm/loongson/lsdc_ttm.c > index bb0c8fd43a75..bf79dc55afa4 100644 > --- a/drivers/gpu/drm/loongson/lsdc_ttm.c > +++ b/drivers/gpu/drm/loongson/lsdc_ttm.c > @@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct drm_device *ddev, > int ret; > > lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL); > + if (IS_ERR(lbo)) > + return ERR_CAST(lbo); > > ret = lsdc_bo_reserve(lbo); > if (unlikely(ret)) {
"drm/loongson: Add a check for lsdc_bo_create() errors" On 2023/7/18 15:01, Dan Carpenter wrote: > The lsdc_bo_create() function can fail so add a check for that. > > Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c b/drivers/gpu/drm/loongson/lsdc_ttm.c > index bb0c8fd43a75..bf79dc55afa4 100644 > --- a/drivers/gpu/drm/loongson/lsdc_ttm.c > +++ b/drivers/gpu/drm/loongson/lsdc_ttm.c > @@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct drm_device *ddev, > int ret; > > lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL); > + if (IS_ERR(lbo)) > + return ERR_CAST(lbo); > > ret = lsdc_bo_reserve(lbo); > if (unlikely(ret)) {
Basically everything in this email was wrong to a kind of shocking degree. For example, ignoring kmalloc() failure is a bug so the fixes tag is definitely warranted. But then you called me "bare brained" which seems like a personal attack so I'm going to report this as a code of conduct violation. regards, dan carpenter On Tue, Jul 18, 2023 at 08:32:02PM +0800, suijingfeng wrote: > Hi, > > > Thanks for the patch. > > > The commit title generally should be 'drm/looongson: Add a check for > lsdc_bo_create() errors' > > not 'drm: loongson: xxxx' > > > On 2023/7/18 15:01, Dan Carpenter wrote: > > The lsdc_bo_create() function can fail so add a check for that. > > > > Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller") > > Please drop this Fixes tag, because you patch just add a error handling. > > Yes, the lsdc_bo_create() function can fail, but this is generally not > happen. > > Even if the fail happened, your patch is not fixing the root problem. > > > I know that you create this patch with the bare brain, > > I would like hear more practical usage or bugs of this driver. > > And mention more in the commit message is preferred. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c b/drivers/gpu/drm/loongson/lsdc_ttm.c > > index bb0c8fd43a75..bf79dc55afa4 100644 > > --- a/drivers/gpu/drm/loongson/lsdc_ttm.c > > +++ b/drivers/gpu/drm/loongson/lsdc_ttm.c > > @@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct drm_device *ddev, > > int ret; > > lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL); > > + if (IS_ERR(lbo)) > > + return ERR_CAST(lbo); > > ret = lsdc_bo_reserve(lbo); > > if (unlikely(ret)) {
People have suggested that I misread this and that "bare brain" means
through code review instead of testing. In context that seems to make
sense.
Sorry.
Anyway, the fixes tag is warranted.
> > Even if the fail happened, your patch is not fixing the root problem.
What is the correct fix then?
regards,
dan carpenter
Hi, On 2023/7/18 21:27, Dan Carpenter wrote: > Basically everything in this email was wrong to a kind of shocking > degree. For example, ignoring kmalloc() failure is a bug so the fixes > tag is definitely warranted. But then you called me "bare brained" > which seems like a personal attack Sorry, that's a misunderstanding Sorry for my broken English. by "bare brain", I means that by using the brains(pure code review) only, I conjure up this adjective from the word "bare metal". When I reply you email, I lack a word to describe this. I believe that many experts have this sort of ability, they could create a patch by simply give a glimpse of the code. because they know how does the code run at the very low level. > so I'm going to report this as a code > of conduct violation. Sorry Dan, you are welcomed. Please withdraw this report. I don't know why are you angry. Because our hardware is rare, Originally, by using the words "bare brain", I means by "pure brain", I means that you probably without a hardware platform to do verification. I want to know that if you have tested your patch on the board. Or, I want to probe that if you have our hardware. I will consider to send you one if you are long time contributor and if you are really interested. I have a lot of boards, now I'm feel a little bit tired by developing drivers for all of them. Please withdraw that report, personal attack tend to continues(across) to multiple thread. Sorry for my broken English. I will improve my written skill in the long term. Thanks for you contribution. > regards, > dan carpenter > > On Tue, Jul 18, 2023 at 08:32:02PM +0800, suijingfeng wrote: >> Hi, >> >> >> Thanks for the patch. >> >> >> The commit title generally should be 'drm/looongson: Add a check for >> lsdc_bo_create() errors' >> >> not 'drm: loongson: xxxx' >> >> >> On 2023/7/18 15:01, Dan Carpenter wrote: >>> The lsdc_bo_create() function can fail so add a check for that. >>> >>> Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller") >> Please drop this Fixes tag, because you patch just add a error handling. >> >> Yes, the lsdc_bo_create() function can fail, but this is generally not >> happen. >> >> Even if the fail happened, your patch is not fixing the root problem. >> >> >> I know that you create this patch with the bare brain, >> >> I would like hear more practical usage or bugs of this driver. >> >> And mention more in the commit message is preferred. >> >> >>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >>> --- >>> drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c b/drivers/gpu/drm/loongson/lsdc_ttm.c >>> index bb0c8fd43a75..bf79dc55afa4 100644 >>> --- a/drivers/gpu/drm/loongson/lsdc_ttm.c >>> +++ b/drivers/gpu/drm/loongson/lsdc_ttm.c >>> @@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct drm_device *ddev, >>> int ret; >>> lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL); >>> + if (IS_ERR(lbo)) >>> + return ERR_CAST(lbo); >>> ret = lsdc_bo_reserve(lbo); >>> if (unlikely(ret)) {
Hi, On 2023/7/18 21:59, Dan Carpenter wrote: > People have suggested that I misread this and that "bare brain" means > through code review instead of testing. In context that seems to make > sense. > > Sorry. Sorry for my broken English, that's really a misunderstanding. > Anyway, the fixes tag is warranted. Okay, I'll accept this if no other experts object. To follow the convention of the DRM world, please rename the commit title by "drm/loongson: Add a check for lsdc_bo_create() errors". Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> With this small problem solved.
Hi, I still remember you are helps to review the drm/lsdc patch one years ago, see [1]. drm/lsdc is the former version of drm/loongson, originally drm/lsdc are embedded SoCs of Loongson. [1] https://patchwork.freedesktop.org/patch/471997/?series=99512&rev=5 I haven't forget about you. On 2023/7/18 21:59, Dan Carpenter wrote: >>> Even if the fail happened, your patch is not fixing the root problem. > What is the correct fix then? lsdc_bo_create_kernel_pinned() is intend to be used when you do the self test (cat benchmark) which is using to testing the hardware bandwidth via debugfs. Another potential usage is to implement built in fbdev emulation. I admit the error handling is necessary, but it's a kind of costuming. To be honest, I have also post similar patches to other DRM drivers.:-) So, that is okay. But let's back to the technique, when the kzalloc() fails, who will survives? If the the kzalloc() fail, then I think the implement of kzalloc() should be blamed first. while developing this driver nearly about two years, it rare happens. For my drivers, kzalloc() fails is one sort of error, lsdc_bo_create() could fail when no enough VRAM is another. The dumb_buffer test if IGT would helps to probe such failures. But this is a kind of limitation of the hardware itself. Its a resource limitation, even the drm/radeon could probably fail. So, How does your patch could resolve the root problems that caused by no enough resource available?
Yeah. Sorry again. I was frustrated. Your email basically said everything was wrong with my patch. The subject was wrong. The commit message was too short. The Fixes tag was wrong. The patch wasn't correct and didn't address the root cause. And then when you mentioned "bare brain" I misunderstood and took it personal. In the end, I think we've all agreed the patch is basically fine. I will change the subject and resend. People are going to send you patches, right? It's not a big deal. You just have to look it over and see if it's useful or not. You should try to respond within a week or two. But even if you apply a bad patch you have a couple months to test it before it goes to users. And even when a bug reaches users, that's unfortunate but it can still be fixed. regards, dan carpenter
diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c b/drivers/gpu/drm/loongson/lsdc_ttm.c index bb0c8fd43a75..bf79dc55afa4 100644 --- a/drivers/gpu/drm/loongson/lsdc_ttm.c +++ b/drivers/gpu/drm/loongson/lsdc_ttm.c @@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct drm_device *ddev, int ret; lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL); + if (IS_ERR(lbo)) + return ERR_CAST(lbo); ret = lsdc_bo_reserve(lbo); if (unlikely(ret)) {
The lsdc_bo_create() function can fail so add a check for that. Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++ 1 file changed, 2 insertions(+)