| Message ID | 20231016204741.1014-2-phil@philpotter.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cdrom: cleanup patch for inclusion | expand |
On 10/16/23 2:47 PM, Phillip Potter wrote: > From: Edson Juliano Drosdeck <edson.drosdeck@gmail.com> > > Add missing blank lines after declarations to fix warning found by > checkpatch.pl script. Let's please not do this. It's fine to run checkpatch on new patches to ensure that you don't make mistakes, but this is just useless churn. Even worse: > @@ -1202,6 +1204,7 @@ static int check_for_audio_disc(struct cdrom_device_info *cdi, > { > int ret; > tracktype tracks; > + > cd_dbg(CD_OPEN, "entering check_for_audio_disc\n"); > if (!(cdi->options & CDO_CHECK_TYPE)) > return 0; This int ret is using spaces and not a tab, why even make a newline change and not sort that out too? But it's all mostly moot as we should not be doing patches like this.
On Mon, Oct 16, 2023 at 02:53:06PM -0600, Jens Axboe wrote: > On 10/16/23 2:47 PM, Phillip Potter wrote: > > From: Edson Juliano Drosdeck <edson.drosdeck@gmail.com> > > > > Add missing blank lines after declarations to fix warning found by > > checkpatch.pl script. > > Let's please not do this. It's fine to run checkpatch on new patches to > ensure that you don't make mistakes, but this is just useless churn. > Even worse: > Hi Jens, So to be clear, I should not accept patches that do cleanup like this in future unless there are other substantive changes? I also build tested the patch as per normal. > > @@ -1202,6 +1204,7 @@ static int check_for_audio_disc(struct cdrom_device_info *cdi, > > { > > int ret; > > tracktype tracks; > > + > > cd_dbg(CD_OPEN, "entering check_for_audio_disc\n"); > > if (!(cdi->options & CDO_CHECK_TYPE)) > > return 0; > > This int ret is using spaces and not a tab, why even make a newline > change and not sort that out too? > Yes, good point. Given the patch only consisted of new lines though, I didn't think it a bad one. If this is the policy though, I will be stricter in future of course. > But it's all mostly moot as we should not be doing patches like this. > > -- > Jens Axboe > Regards, Phil
On 10/16/23 3:07 PM, Phillip Potter wrote: > On Mon, Oct 16, 2023 at 02:53:06PM -0600, Jens Axboe wrote: >> On 10/16/23 2:47 PM, Phillip Potter wrote: >>> From: Edson Juliano Drosdeck <edson.drosdeck@gmail.com> >>> >>> Add missing blank lines after declarations to fix warning found by >>> checkpatch.pl script. >> >> Let's please not do this. It's fine to run checkpatch on new patches to >> ensure that you don't make mistakes, but this is just useless churn. >> Even worse: >> > Hi Jens, > > So to be clear, I should not accept patches that do cleanup like this > in future unless there are other substantive changes? I also build > tested the patch as per normal. Right. May be fine if actual fixes are being made, then do it as a prep patch or something. That sometimes happens, you fix something and notice that some styling is off and then fix that too. >>> @@ -1202,6 +1204,7 @@ static int check_for_audio_disc(struct cdrom_device_info *cdi, >>> { >>> int ret; >>> tracktype tracks; >>> + >>> cd_dbg(CD_OPEN, "entering check_for_audio_disc\n"); >>> if (!(cdi->options & CDO_CHECK_TYPE)) >>> return 0; >> >> This int ret is using spaces and not a tab, why even make a newline >> change and not sort that out too? >> > > Yes, good point. Given the patch only consisted of new lines though, I > didn't think it a bad one. If this is the policy though, I will be > stricter in future of course. It's just pointless churn, which is the objection. Now granted for cdrom the risk of conflict isn't that large, as it doesn't really see any changes. But in general this is the kind of stuff that prevents stable backports from just applying automatically. I view it similarly to spelling fixes, and that kind of stuff. Is the style wrong? It is. But it's not worth fixing seperately.
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index a5e07270e0d4..d1c2c1095fdd 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -985,6 +985,7 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks) struct cdrom_tochdr header; struct cdrom_tocentry entry; int ret, i; + tracks->data = 0; tracks->audio = 0; tracks->cdi = 0; @@ -1038,6 +1039,7 @@ int open_for_data(struct cdrom_device_info *cdi) int ret; const struct cdrom_device_ops *cdo = cdi->ops; tracktype tracks; + cd_dbg(CD_OPEN, "entering open_for_data\n"); /* Check if the driver can report drive status. If it can, we can do clever things. If it can't, well, we at least tried! */ @@ -1202,6 +1204,7 @@ static int check_for_audio_disc(struct cdrom_device_info *cdi, { int ret; tracktype tracks; + cd_dbg(CD_OPEN, "entering check_for_audio_disc\n"); if (!(cdi->options & CDO_CHECK_TYPE)) return 0; @@ -3038,6 +3041,7 @@ static noinline int mmc_ioctl_cdrom_subchannel(struct cdrom_device_info *cdi, int ret; struct cdrom_subchnl q; u_char requested, back; + if (copy_from_user(&q, (struct cdrom_subchnl __user *)arg, sizeof(q))) return -EFAULT; requested = q.cdsc_format; @@ -3063,6 +3067,7 @@ static noinline int mmc_ioctl_cdrom_play_msf(struct cdrom_device_info *cdi, { const struct cdrom_device_ops *cdo = cdi->ops; struct cdrom_msf msf; + cd_dbg(CD_DO_IOCTL, "entering CDROMPLAYMSF\n"); if (copy_from_user(&msf, (struct cdrom_msf __user *)arg, sizeof(msf))) return -EFAULT; @@ -3083,6 +3088,7 @@ static noinline int mmc_ioctl_cdrom_play_blk(struct cdrom_device_info *cdi, { const struct cdrom_device_ops *cdo = cdi->ops; struct cdrom_blk blk; + cd_dbg(CD_DO_IOCTL, "entering CDROMPLAYBLK\n"); if (copy_from_user(&blk, (struct cdrom_blk __user *)arg, sizeof(blk))) return -EFAULT; @@ -3177,6 +3183,7 @@ static noinline int mmc_ioctl_cdrom_start_stop(struct cdrom_device_info *cdi, int cmd) { const struct cdrom_device_ops *cdo = cdi->ops; + cd_dbg(CD_DO_IOCTL, "entering CDROMSTART/CDROMSTOP\n"); cgc->cmd[0] = GPCMD_START_STOP_UNIT; cgc->cmd[1] = 1; @@ -3190,6 +3197,7 @@ static noinline int mmc_ioctl_cdrom_pause_resume(struct cdrom_device_info *cdi, int cmd) { const struct cdrom_device_ops *cdo = cdi->ops; + cd_dbg(CD_DO_IOCTL, "entering CDROMPAUSE/CDROMRESUME\n"); cgc->cmd[0] = GPCMD_PAUSE_RESUME; cgc->cmd[8] = (cmd == CDROMRESUME) ? 1 : 0; @@ -3230,6 +3238,7 @@ static noinline int mmc_ioctl_dvd_auth(struct cdrom_device_info *cdi, { int ret; dvd_authinfo ai; + if (!CDROM_CAN(CDC_DVD)) return -ENOSYS; cd_dbg(CD_DO_IOCTL, "entering DVD_AUTH\n"); @@ -3248,6 +3257,7 @@ static noinline int mmc_ioctl_cdrom_next_writable(struct cdrom_device_info *cdi, { int ret; long next = 0; + cd_dbg(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n"); ret = cdrom_get_next_writable(cdi, &next); if (ret) @@ -3262,6 +3272,7 @@ static noinline int mmc_ioctl_cdrom_last_written(struct cdrom_device_info *cdi, { int ret; long last = 0; + cd_dbg(CD_DO_IOCTL, "entering CDROM_LAST_WRITTEN\n"); ret = cdrom_get_last_written(cdi, &last); if (ret)