| Message ID | 20231122161304.12434-1-ghanshyam1898@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] media: stk1160: Fixed high volume of stk1160_dbg messages | expand |
On Wed, Nov 22, 2023 at 09:43:04PM +0530, Ghanshyam Agrawal wrote: > The function stk1160_dbg gets called too many times, which causes > the output to get flooded with messages. Since stk1160_dbg uses > printk, it is now replaced with dev_warn_ratelimited. > > Suggested-by: Phillip Potter <phil@philpotter.co.uk> > Signed-off-by: Ghanshyam Agrawal <ghanshyam1898@gmail.com> > --- > v2: > Thanks for your suggestions Phillip. I have updated the TODO comment and > used dev_warn_ratelimited for inclusion of kernel warning. > > drivers/media/usb/stk1160/stk1160-video.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c Hi Ghanshyam, Thank you for the patch, but it is sadly incorrect. You have created this V2 against a tree including the V1 version of your patch. A V2 patch should apply cleanly against the source tree, with no previous version first needing to be applied. On another note, why are you using dev_warn_ratelimited here, and if there's a good reason, why not use it for the other callsites in this function? (Genuine question here, I've not studied the difference). Please create a V3, many thanks. Nacked-by: Phillip Potter <phil@philpotter.co.uk> Regards, Phil
On Sat, Nov 25, 2023 at 4:00 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > On Wed, Nov 22, 2023 at 09:43:04PM +0530, Ghanshyam Agrawal wrote: > > The function stk1160_dbg gets called too many times, which causes > > the output to get flooded with messages. Since stk1160_dbg uses > > printk, it is now replaced with dev_warn_ratelimited. > > > > Suggested-by: Phillip Potter <phil@philpotter.co.uk> > > Signed-off-by: Ghanshyam Agrawal <ghanshyam1898@gmail.com> > > --- > > v2: > > Thanks for your suggestions Phillip. I have updated the TODO comment and > > used dev_warn_ratelimited for inclusion of kernel warning. > > > > drivers/media/usb/stk1160/stk1160-video.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c > > Hi Ghanshyam, > > Thank you for the patch, but it is sadly incorrect. > > You have created this V2 against a tree including the V1 version of your > patch. A V2 patch should apply cleanly against the source tree, with no > previous version first needing to be applied. > > On another note, why are you using dev_warn_ratelimited here, and if > there's a good reason, why not use it for the other callsites in this > function? (Genuine question here, I've not studied the difference). > > Please create a V3, many thanks. > > Nacked-by: Phillip Potter <phil@philpotter.co.uk> > > Regards, > Phil Hi Phillip, Thank you for taking time to review my patch. I will fix the issue with my patch needing previous versions being applied first. I had used dev_warn_ratelimited because the checkpatch script mentioned a sequence of priorities to use various logging functions and dev_warn_ratelimited had higher priority. But now I have found other issues with using this function here, so I will switch back to printk_ratelimited. Let me fix the issues and submit a V3 patch. Thanks again for your time and kind suggestions. Regards, Ghanshyam Agrawal
diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c index f5b75f380c19..a8e0d352a68c 100644 --- a/drivers/media/usb/stk1160/stk1160-video.c +++ b/drivers/media/usb/stk1160/stk1160-video.c @@ -105,6 +105,16 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) u8 *dst = buf->mem; int remain; + /* + * TODO: These stk1160_dbg are very spammy! + * We should check why we are getting them. + * + * UPDATE: One of the reasons (the only one?) for getting these + * is incorrect standard (mismatch between expected and configured). + * So perhaps, we could add a counter for errors. When the counter + * reaches some value, we simply stop streaming. + */ + len -= 4; src += 4; @@ -140,7 +150,7 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len) /* Let the bug hunt begin! sanity checks! */ if (lencopy < 0) { - printk_ratelimited("copy skipped: negative lencopy\n"); + dev_warn_ratelimited(dev, "copy skipped: negative lencopy\n"); return; }
The function stk1160_dbg gets called too many times, which causes the output to get flooded with messages. Since stk1160_dbg uses printk, it is now replaced with dev_warn_ratelimited. Suggested-by: Phillip Potter <phil@philpotter.co.uk> Signed-off-by: Ghanshyam Agrawal <ghanshyam1898@gmail.com> --- v2: Thanks for your suggestions Phillip. I have updated the TODO comment and used dev_warn_ratelimited for inclusion of kernel warning. drivers/media/usb/stk1160/stk1160-video.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)