Re: Kernel bug tracker
Hi, > 1. When submitting the patch should I include you in the copy > as the original author? I guess this is more a question for the experienced patch submitters. My cheat sheet points to https://www.kernel.org/doc/html/v5.10/process/submitting-patches.html The code change is trivial enough that i do not claim authorship. So how about Suggested-by: Thomas Schmitt I found the bug and suggested an obvious fix. The most kernel merit i can claim is that i grepped through the kernel sources for callers of iso_date() and found them all ready for the change of result type. So maybe Co-developed-by: Thomas Schmitt Signed-off-by: Thomas Schmitt But on the other hand you should verify my claims in -cover-letter.patch before posting your patch. A year has passed since i did my research and testing. So my statements create not more authorship than a detailed bug report would do. Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Kernel bug tracker
Hi, maybe i should not have pasted my patches into a new mail. My mail client shows the first mail as three mails. Possibly an effect of the mailbox-like format which it got by pasting in two send-ready git patches. Strangely it shows the second mail with the Rock Ridge patch as a single one. Sorry for any confusion in the receiving mail boxes. Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Kernel bug tracker
Hi, second patch proposal for isofs because Adverg Ebashinskii wrote: > If you could share your patch here to understand the problem better I would > gladly dig into it. -cover-letter.patch From 3d484405f0ad8d10ef490281da157bfdd7450cb6 Mon Sep 17 00:00:00 2001 From: Thomas Schmitt Date: Tue, 22 Sep 2020 12:35:52 +0200 Subject: [PATCH 0/1] isofs: truncate oversized Rock Ridge names to 255 bytes Currently Rock Ridge names of length >= 254 are coarsely truncated by discarding the whole NM entry where the overflow happened. This yields name lengths of much less than the permissible 255 bytes. There is no reason to see why to exclude length 254 and 255 and especially to truncate by possibly a hundred or more bytes than necessary. So i propose to raise the length of permissible names to 255 and to let truncation yield exactly a string length of 255 bytes. Truncation shall take care to invalidate UTF-8 debris at the end of the resulting string (sorry ISO-8859). --- Tests made: Create an ISO 9660 image with file names of length 255, using file /bin/true as input for both files: victim1=12345678901234567890123456789012345678901234567890 victim1="$victim1"12345678901234567890123456789012345678901234567890 victim1="$victim1"12345678901234567890123456789012345678901234567890 victim1="$victim1"12345678901234567890123456789012345678901234567890 victim1="$victim1"12345678901234567890123456789012345678901234567890 victim1="$victim1"1234E victim2=ä victim2="$victim2"ä victim2="$victim2"ä victim2="$victim2"ä victim2="$victim2"ä victim2="$victim2"xää xorriso -outdev /tmp/test_rr_name.iso \ -blank as_needed \ -map /bin/true /"$victim1" \ -map /bin/true /"$victim2" Currently the names get truncated to byte lengths 93 and 95: mount /tmp/test_rr_name.iso /mnt/iso /bin/ls /mnt/iso yields in xterm with bash 12345678901234567890...60.more.bytes...1234567890123 'ää'$'\303' Note the leading blank with the plain ASCII name and the shell characters with the name that has 2-byte UTF-8 characters. But /bin/ls /mnt/iso | cat yields 12345678901234567890...60.more.bytes...1234567890123 ää� The extra characters in xterm seem to be triggered by the presence of the half UTF-8 'ä' at the end. Its byte 0xc3 is there, byte 0xa4 is missing. (xterm and /bin/ls are from Debian 10.) If i make the UTF-8 name shorter to avoid truncation or if i move the 'x' to the start to cause truncation between complete UTF-8 'ä', the extra characters do not show up in ls to xterm. After my change in fs/isofs i get from /bin/ls /mnt/iso 1234567890...230.more.bytes...12345678901234E ää...210.more.bytes...ääxää Both strings have 255 bytes. xorriso cannot be talked into writing longer Rock Ridge names. So i rather set the new macro RR_NAME_LEN in rock.h to 33 to force truncation. The result with /bin/ls -1 /mnt/iso is: 123456789012345678901234567890123 _ Note the half 'ä' at the end being mapped to '_'. So all characters are valid UTF-8 and no oddities of ls or xterm are to see. --- Remaining checkpatch.pl warning: scripts/checkpatch.pl complains about the string 'ää'$'\303' in this text by: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) Maybe it should talk about "bytes" rather than "chars" or learn about multi-byte characters in UTF-8. I think it is beneficial if i show the whole mangled name, rather than describing it by some ASCII-only text. --- Have a nice day :) Thomas Thomas Schmitt (1): isofs: truncate oversized Rock Ridge names to 255 bytes fs/isofs/rock.c | 73 ++--- fs/isofs/rock.h | 2 ++ 2 files changed, 71 insertions(+), 4 deletions(-) -- 2.20.1 0001-isofs-truncate-oversized-Rock-Ridge-names-to-255-byt.patch From 3d484405f0ad8d10ef490281da157bfdd7450cb6 Mon Sep 17 00:00:00 2001 From: Thomas Schmitt Date: Tue, 22 Sep 2020 12:34:50 +0200 Subject: [PATCH 1/1] isofs: truncate oversized Rock Ridge names to 255 bytes Enlarge the limit for name bytes from 253 to 255. Do not discard
Re: Kernel bug tracker
Hi,
Adverg Ebashinskii wrote:
> If you could share your patch here to understand the problem better I would
> gladly dig into it.
Ok. Here is the simple one. The other comes in a separate mail.
The following texts stem from git format-patch. If submitting for real,
i would send them by git send-email to linux-ker...@vger.kernel.org and
linux-s...@vger.kernel.org.
(The latter because Jens Axboe committed a few isofs changes in the past
and because isofs is historically related to sr and cdrom.)
-cover-letter.patch
>From 154a68527351db091e5de60388ba4cfb1fe779fd Mon Sep 17 00:00:00 2001
From: Thomas Schmitt
Date: Mon, 21 Sep 2020 18:20:14 +0200
Subject: [PATCH 0/1] isofs: prevent file time rollover after year 2038
The time values in struct inode of isofs result from calls to function
iso_date() in isofs/util.c, which returns seconds in the range of signed
int. This will rollover in 2038.
ISO 9660 directory record timestamps are good for up to year 2155.
(ECMA-119 9.1.5: 1900 + 255)
The only callers of iso_date() are in isofs/inode.c and isofs/rock.c
and put the result into struct inode.i_{a,c,m}time.tv_sec which is
of type time64_t.
The time value of iso_date() essentially stems from mktime64().
So return type time64_t is appropriate for iso_date().
--
Demonstration of the problem:
Create an ISO 9660 filesystem with file date in 2040, using file /bin/true
as victim payload:
xorriso -outdev /tmp/test_date.iso \
-blank as_needed \
-map /bin/true /victim \
-alter_date m 'Oct 01 22:06:12 2040' /victim --
Inspect the current representation by isofs:
mount /tmp/test_date.iso /mnt/iso
ls -l /mnt/iso/victim
This yields with int iso_date():
... Aug 26 1904 /mnt/iso/victim
After changing the type of iso_date() to time64_t:
... Oct 1 2040 /mnt/iso/victim
For completeness i tested the last possible second:
xorriso ... -alter_date m 'Dec 31 23:59:59 2155' /victim --
and got properly:
... Dec 31 2155 /mnt/iso/victim
(When reproducing this it might be to wise to use December 30, to avoid
any potential timezone problems.)
--
Have a nice day :)
Thomas
Thomas Schmitt (1):
isofs: prevent file time rollover after year 2038
fs/isofs/isofs.h | 3 ++-
fs/isofs/util.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
--
2.20.1
0001-isofs-prevent-file-time-rollover-after-year-2038.patch
>From 154a68527351db091e5de60388ba4cfb1fe779fd Mon Sep 17 00:00:00 2001
From: Thomas Schmitt
Date: Mon, 21 Sep 2020 18:20:06 +0200
Subject: [PATCH 1/1] isofs: prevent file time rollover after year 2038
Change the return type of function iso_date() from int to time64_t.
Signed-off-by: Thomas Schmitt
---
fs/isofs/isofs.h | 3 ++-
fs/isofs/util.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
index 055ec6c586f7..527c0db72ff9 100644
--- a/fs/isofs/isofs.h
+++ b/fs/isofs/isofs.h
@@ -107,7 +107,8 @@ static inline unsigned int isonum_733(u8 *p)
/* Ignore bigendian datum due to broken mastering programs */
return get_unaligned_le32(p);
}
-extern int iso_date(u8 *, int);
+
+time64_t iso_date(u8 *, int);
struct inode; /* To make gcc happy */
diff --git a/fs/isofs/util.c b/fs/isofs/util.c
index e88dba721661..348af786a8a4 100644
--- a/fs/isofs/util.c
+++ b/fs/isofs/util.c
@@ -16,10 +16,10 @@
* to GMT. Thus we should always be correct.
*/
-int iso_date(u8 *p, int flag)
+time64_t iso_date(u8 *p, int flag)
{
int year, month, day, hour, minute, second, tz;
- int crtime;
+ time64_t crtime;
year = p[0];
month = p[1];
--
2.20.1
Have a nice day :)
Thomas
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Kernel bug tracker
Hi, Valdis Klētnieks wrote: > The tricky part is, of course, that for this to work correctly, you need > to have 64-bit timestamps in the on-disk format. Initially yes. In https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=800627 i sketched what it thought was needed to do. But by the much more elegant https://github.com/torvalds/linux/commit/34be4dbf87fc the full ISO 9660 date range up to year 2155 would be correctly shown, if not in year 2038 signed int would roll over. Demo: xorriso -outdev /tmp/test_date.iso \ -blank as_needed \ -map /bin/true /victim \ -alter_date m 'Oct 01 22:06:12 2040' /victim -- mount /tmp/test_date.iso /mnt/iso ls -l /mnt/iso/victim yields currently ... Aug 26 1904 /mnt/iso/victim But after the really simple change to time64_t it yields ... Oct 1 2040 /mnt/iso/victim So this is really a low hanging fruit in fs. Still there today in the torvalds Github repo. > > - isofs: truncate oversized Rock Ridge names to 255 bytes > > Map trailing incomplete UTF-8 bytes to '_'. > A better answer would probably be to truncate it at the last complete UTF-8 > that leaves the string at 255 or less. My patch proposal could be changed accordingly. But with '_' as placeholders of bytes from incomplete UTF-8 characters there would be a distinction to names with the same start bytes but ending directly before the UTF-8 character which got cut apart. The need for real truncation should rarely occur. Main motivation for fixing this would be this observation: Currently Rock Ridge names of length >= 254 are coarsely truncated by discarding the whole NM entry where the overflow happened. This yields name lengths of much less than the permissible 255 bytes. There is no reason to see why to exclude length 254 and 255 and especially to truncate by possibly a hundred or more bytes than necessary. File names in ISO 9660 + Rock Ridge ISO 1234567890...230.more.bytes...12345678901234E ää...210.more.bytes...ääxää get shown after mount(8) in xterm with bash by /bin/ls as 12345678901234567890...60.more.bytes...1234567890123 'ää'$'\303' Note the leading blank with the plain ASCII name and the shell characters with the name that has 2-byte UTF-8 characters. (Rock Ridge encodes its names in one or more NM entries. Long names often get split between a NM in the file's ISO 9660 directory record and a NM in the Contiuation Area of the file. That second one gets dropped.) Other than the time rollover fix, this problem needs some knowledge about ISO 9660, which is available for free as ECMA-119, and about SUSP + RRIP of which specs are available for free too. Both are really simple, compared with e.g. UDF specs. I am ready to explain in detail what is neded to understand the problem. If Adverg Ebashinskii wants, i'll hand over my patch as guideline, or as base for own work, or just for review, testing, and posting. I can give instructions how to reproduce each of the three bugs by help of small ISO images made with xorriso. Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Kernel bug tracker
Hi, Adverg Ebashinskii wrote: > The reason I looked for some bugs is that I’m not > really interested in driver development and digging into details of a > specific hardware. So I tried to get into some core subsystems like fs, net, > cgroups, etc... I could offer bugs of isofs with explanations and patch proposals: - isofs: prevent file time rollover after year 2038 Change the return type of function iso_date() from int to time64_t. - isofs: truncate oversized Rock Ridge names to 255 bytes Do not discard all bytes of the NM field where the overflow occurs, but rather append them to the accumulated name before truncating it to exactly 255 bytes. Map trailing incomplete UTF-8 bytes to '_'. - isofs: fix Oops with zisofs and large PAGE_SIZE https://lore.kernel.org/linux-scsi/20201120140633.1673-1-scdbac...@gmx.net/T/#u (No replies since 2020-11-20. I hope the tester of this patch still has the machine to confirm that the patch is still good.) What is obviously missing with my skills is ability to get attention of kernel developers for isofs and their trust that the proposals don't make things worse. As developer of libisofs and libburn i can provide motivations and facts from that experience. There would be 4 bugs in cdrom and sr to be fixed, 2 wishlist changes for them, and 2 wishlist changes for isofs. An example can be seen at https://lore.kernel.org/linux-scsi/20201006094026.1730-1-scdbac...@gmx.net/T/#u (My patch proposals were tested with kernels of a year ago. One of them meanwhile needs rework due to the demise of the .readpages method: isofs: Give zisofs a .readpages() method for use by mm/readahead My kernel development machine from then meanwhile has a production job.) Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: What to do when a patch set gets no reply for 3 weeks ?
Hi, Greg KH wrote: > repost it and don't worry about it, maintainers get behind. Will do. > Also, I think the scsi list wants developers to help review other > patches, right? Why not help out with that while you wait for your > patches to be reviewed? My SCSI knowledge is concentrated on optical drives, which i am used to operate from userland via ioctl(SG_IO). So if it's not about cdrom or sr (or isofs), i am not qualified to have an own opinion. I saw no patch sets which would deal with my topics, other than the very broad one by Christoph Hellwig which by two patches affects sr: https://lore.kernel.org/linux-scsi/b5e51c24-b9a6-979f-8fe0-f762f113b...@kernel.dk/T/#t I studied it, but the only thought it gave me was that my wished new ioctl had to become more independent of the entrails of general block device handling. (Now it is much leaner and less needy of justifications that it is indeed harmless.) Hm. Jens Axboe writes "Applied". Maybe i can learn to which repo and rebase my patch for that one ... Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
What to do when a patch set gets no reply for 3 weeks ?
Hi, i sent my first patch set three weeks ago. See: https://lore.kernel.org/linux-scsi/20200908120207.5014-1-scdbac...@gmx.net/T/#u It was made with git format-patch and sent by git send-email to the addresses which were proposed here. In linux-scsi it showed up after a day. Then i followed the advise in Documentation/process/submitting-patches.rst "Wait for a minimum of one week before resubmitting or pinging reviewers - possibly longer" Still no reply yet. Neither to my mail address nor visible in the thread display of lore.kernel.org. Could some merciful person have a look at my patch set and point me to obvious reasons why it does not get considered ? -- If no obvious shortcomming disqualifies my patches, what to do next ? - Ping everybody ? - Repost ? - Post one of the patch sets which i made during my time of patience ? I have - 3 bug fixes for cdrom+sr - 1 augmentation of cdrom+sr for multi-session DVD and BD media - 1 wish for a new ioctl CDROM_SIMUL_CHANGE in cdrom+sr - 2 bug fixes for isofs Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Please give advise about my first patch attempt
Hi, i'm back at work with my patch for automatic CD loading, but the target moves faster than i can follow. Meanwhile the sr part of my change plays on a fresh construction site of linux-scsi: https://lore.kernel.org/linux-scsi/730eced4-c804-a78f-3d52-2a448dbd1...@interlog.com/T/#t "rework check_disk_change()" 2020-09-02 14:11 Christoph Hellwig Currently our plans overlap by [PATCH 17/19] "sr: use bdev_check_media_change" although they do not (yet) really conflict https://lore.kernel.org/linux-scsi/730eced4-c804-a78f-3d52-2a448dbd1...@interlog.com/T/#m9d4c4b6145ed41d6467f2dc4728d6fd0345e94f1 It seems unwise to propose changes while Christoph Hellwig is actively working on the same code. (And i don't even know where to get the git branch with such a recent change when it is committed.) What is the socially acceptable way to coordinate plans with him ? - Shall i patiently wait until he is done with cdrom and sr ? - Shall i announce my interest in fixing automatic tray loading and other things in a brief mail to the list ? - Shall i post my patches (*) as they are and just wait for instructions ? (*) Studying Christoph's work in linux-scsi brought me to the prediction that they will want separate patches for cdrom and sr. Also i reworked my implementation along his design pattern of factoring out and exporting functions and then calling them where beneficial. (I.e. my -EDEADLK hack is dead. Long live cdrom_handle_open_tray(). Larger cdrom patch, but also nicer looking code.) Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Please give advise about my first patch attempt
Hi, i might get a community problem with the goal of my patch. While googling for hints of acceptance for cdrom on linux-s...@vger.kernel.org i came to a thread where a much more expensive fix of the problem was obviously rejected by Jens Axboe in november 2019: https://lore.kernel.org/linux-scsi/c6fe572c-530e-93eb-d62a-cb2f89c7b...@kernel.dk/ "I think the main complaint with this is that it's kind of a stretch to add core functionality for a device type that's barely being manufactured anymore and is mostly used in a virtualized fashion. I think it you could fix this without 10 patches of churn and without adding a new ->open() addition to fops, then people would be a lot more receptive to the idea of improving cdrom auto-close." Well, 10 patches. Each about the code size of my single one. But also a negative opinion towards the device class by the maintainer. That proposal fixes tray loading for audio, too. I did not care because i cannot test realistically. Then there are patches 04/10 and 08/10 which shall avoid blocking other processes from inquiring the drive while it is loading its tray because of the automatic load feature in open_for_data(). Mine has a 40 seconds timeout instead, blocking CDROM ioctls and sr_bdops.open() == sr_block_open() to that particular drive. Would it be wise to mention from the beginning that i studied that proposal ? My opinion is now that if concurrent access from multiple threads to a drive is more important than a working automatic tray load on open(2), then automatic tray loading should be disabled at all. If concurrent drive inspection by multiple threads at any time is not a main goal, then i would still advertise my patch. Should i mention this opinion from the beginning ? I have a few other cdrom+sr fixes in my local 4.19 kernel. I don't want to spoil their chances by starting with a non-starter. Any strategic proposals to not appear clueless in front of Jens Axboe would be welcome. Due to real life issues i will probably until mid of next week not be able to post a patch on linux-scsi and to then react swiftly on demanding requests. --- Lukas Bulwahn wrote: > Archives at https://lore.kernel.org/ can give you the raw text and that > archive is tested among kernel maintainers for picking patches. > You can prepare your patch and send it to kernelnewbies. > Then, pick it from https://lore.kernel.org/kernelnewbies/ and try to apply > your own patch with git am. If that works, it is probably fine. Pawan Gupta wrote: > $ ./scripts/checkpatch.pl --strict --codespell -g > $ make CFLAGS_KERNEL+=-Werror ... I surely learn a lot here. Already lagging behind now ... :) Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Please give advise about my first patch attempt
Hi,
Lukas Bulwahn wrote:
> Just take all maintainers as recipients.
> ax...@kernel.dk linux-ker...@vger.kernel.org j...@linux.ibm.com
> martin.peter...@oracle.com linux-s...@vger.kernel.org
Really that loudly ?
Ain't linux-ker...@vger.kernel.org too general ?
> It is probably best to base it on v5.9-rc2
Done. The patch was accepted by git apply. git diff looks good.
(There were indeed changes in cdrom.c and cdrom.h. It's not that dead. :))
It compiles, but does not boot:
[1.099627] nvme nvme0: failed to set APST feature (-10)
Gave up waiting for root filesystem device
I booted the 5.8 kernel and was happy to see my SSD well and alive.
Then i reverted my changes, compiled and installed the original 5.9-rc2.
Same boot failure (it would have been astonishing if not).
So i cannot test on 5.9-rc2.
(Had to manually rename vmlinuz-5.9.0-rc2-ts and initrd.img-5.9.0-rc2-ts
before update-grub was willing to create a .cfg which does not boot it
by default. Eww ... whenever i leave the trodden path ...)
> check if it cleanly applies on the latest linux-next
I read
https://www.kernel.org/doc/man-pages/linux-next.html
and did without really knowing why:
$ git remote add linux-next
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
$ git fetch linux-next
...
* [new branch]akpm -> linux-next/akpm
* [new branch]akpm-base -> linux-next/akpm-base
* [new branch]master-> linux-next/master
* [new branch]pending-fixes -> linux-next/pending-fixes
* [new branch]stable-> linux-next/stable
* [new tag] next-20200827 -> next-20200827
$ git checkout master
...
$ git remote update
...
Fetching linux-next
$ git checkout -B linux-next-20200827-ts-issue-2 next-20200827
Switched to a new branch 'linux-next-20200827-ts-issue-2'
$ git apply /home/thomas/v5.9-rc2_issue_2.git.diff
$
Compilation succeeds and it boots.
dd if=/dev/sr0 bs=2048 count=1 of=/dev/null
dd if=/dev/sr1 bs=2048 count=1 of=/dev/null
both succeed starting from open tray with a readable medium in it.
So shall i base my patch on next-20200827 ?
-
> > Please also tell me if my mailer (used with this mail) would cause
> > problems.
> I suggest [...] git send-email ...
I am not sure whether i can get git send-email to work due to local
network issues. My free software mails go out by a primitive SMTP client
which gets fed with plain text files, usually created by vim.
Thus i ask whether my mails show any properties which would hamper
acceptance of a patch, which i would generate by git format-patch.
(I look at patches in https://marc.info/?l=linux-scsi and will try to
mimick them in my text file as good as possible.)
> I cannot comment on the technical stuff, but that what the mailing list is
> good for
Shall i mention that i am developer of libburn since 2006, which on Linux
operates optical drives from userspace via ioctl(SG_IO) ?
> maybe you are the
> next maintainer for CDROM drivers when you continue to test CDROM :)
Urm. This risk exists by having 7 more cdrom and sr problems fixed in
my local production kernel 4.19, plus 2 of fs/iso9660.
But my clumsiness with community and git will hopefully prevent that.
In general it is not easy to test CDROM beyond sr, because the non-sr
devices are very outdated and need bus hardware which i don't have
any more since years. But sr drives are capricious enough to be fun.
-
Garrit Franke wrote:
> In a recent Patch of mine, Greg nicely pointed out that commits should
> have a standard format across mailing lists: sha ("Commit Message").
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2235132.html
Applied to my commit message draft. Thanks for pointing out.
Greg KH wrote:
> That is also documented in the ever-growing submitting patches document
In next-20200827 it's line 930 of Doc*/process/submitting-patches.rst .
I now plan to write:
Commit 210ba1d1724f ("[SCSI] sr: update to follow tray status correctly")
of january 2008 switched sr_drive_status() away from indirectly using
...
By commit 96bcc722c47d ("[SCSI] sr: report more accurate drive status
after closing the tray.") of july 2008 it now returns CDS_DRIVE_NOT_READY
...
Have a nice day :)
Thomas
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Please give advise about my first patch attempt
Hi,
i am preparing my first patch.
May i ask for review and advise how to make it acceptable ?
The patch shall fix an old regression of cdrom and sr drivers.
Although drivers/cdrom has no mailing list entry in MAINTAINERS, i assume
that such patches should go to linux-s...@vger.kernel.org which is listed
for drivers/scsi/sr*. Right ?
So the change affects two different subsystems which have the same maintainer
and have a very close relation. Do i need to split it into two, nevertheless ?
The change is relative to git tag v5.8 of
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
It is not locally committed yet, so i cannot run git format-patch for now.
I plan to do if i get some approval here.
I tested with three different optical drives at SATA and USB by
dd if=/dev/sr0 bs=2048 count=1 of=/dev/null
Unchanged yields ENOMEDIUM or EIO. Changed yields success.
Please also tell me if my mailer (used with this mail) would cause problems.
--
Affected files:
drivers/cdrom/cdrom.c
drivers/scsi/sr.c
include/linux/cdrom.h
Check passed:
scripts/checkpatch.pl
reports
total: 0 errors, 0 warnings, 131 lines checked
--
Intended commit message:
--
Re-enable waiting for CD drive to complete medium decision after tray load
If
open("/dev/sr0", O_RDONLY);
pulls in the tray of an optical drive, it immediately returns -1 with
errno ENOMEDIUM or the first read(2) fails with EIO. Later, when the drive
has stopped blinking, another open() yields success and read() works.
This affects not only userland reading of the device file but also mounting
the device.
Commit 210ba1d1724f5c4ed87a2ab1a21ca861a915f734 of january 2008 switched
sr_drive_status() away from indirectly using sr_do_ioctl() for sending
TEST UNIT READY commands. sr_do_ioctl() has a wait-and-retry loop for
the drive's intermediate sense reply:
2 04 01 Logical unit is in process of becoming ready
Since then sr_drive_status() does not wait any more.
By commit 96bcc722c47d07b6fd05c9d0cb3ab8ea5574c5b1 of july 2008 it now
returns CDS_DRIVE_NOT_READY if above drive reply is received.
But the function open_for_data() in drivers/cdrom/cdrom.c expects that
its call of cdo->drive_status() waits until the drive has decided about
the usability of the inserted medium. Any reply other than CDS_DISC_OK
yields ENOMEDIUM.
Even if the drive waits with its reply to START/STOP UNIT until it is
ready (e.g. Pioneer BDR-S09), the first read(2) yields EIO, because the
newly loaded medium gets not properly assessed by check_disk_change()
before sr_block_open() returns.
Fix the problem by a new function wait_for_medium_decision() which gets
called by open_for_data() instead of directly calling cdo->drive_status().
Make sure that scsi/sr.c assesses the newly loaded medium without additional
risk of deadlock.
After loading the tray and due waiting for readiness, cdrom_open() now returns
-EDEADLK to urge its caller to drop its mutex, to re-run check_disk_change(),
and then to call cdrom_open() again. Only scsi/sr.c is for now aware of this.
cdrom/gdrom.c, paride/pcd.c, and ide/ide-cd.c call cdrom_open() too, but i
cannot test them. So their unchanged code will return -EDEADLK instead of
retrying. Not worse or more deceiving than -ENOMEDIUM or -EIO.
Signed-off-by: Thomas Schmitt
--
Code change:
--
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index d82b3b7658bd..9e113b0dfc82 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -286,6 +286,18 @@
#include
#include
+/*
+ * For the wait-and-retry loop after possibly having loaded the drive tray.
+ * 10 retries in 20 seconds are hardcoded in sr_do_ioctl() which was used
+ * up to 2008.
+ * But time spans up to 25 seconds were measured by libburn on
+ * drives connected via SATA or USB-SATA bridges.
+ * So 20 retries * 2000 ms = 40 seconds seems more appropriate.
+ */
+#define CD_OPEN_MEDIUM_RETRY_MAX 20
+#define CD_OPEN_MEDIUM_RETRY_MSLEEP 2000
+#include
+
/* used to tell the module to turn on full debugging messages */
static bool debug;
/* default compatibility mode */
@@ -1040,10 +1052,38 @@ static void cdrom_count_tracks(struct cdrom_device_info
*cdi, tracktype *tracks)
tracks->cdi, tracks->xa);
}
+static
+int wait_for_medium_decision(struct cdrom_device_info *cdi)
+{
+ int ret, retry = 0;
+ const struct cdrom_device_ops *cdo = cdi->ops;
+
+ /* Wait until the intermediate drive status CDS_DRIVE_NOT_READY ends */
+ while (1) {
+ ret = cdo->drive_status(cdi, CDSL_CURRENT);
+ if (ret == CDS_DRIVE_NOT_R
Does "i_mutex" in comments mean inode_lock() in code ?
Hi, is function inode_lock() the thing which controls what lots of comments call "i_mutex" ? I am in particular riddling over this comment in include/linux/fs.h : /* * NOTE: unlike i_size_read(), i_size_write() does need locking around it * (normally i_mutex), otherwise on 32bit/SMP an update of i_size_seqcount * can be lost, resulting in subsequent i_size_read() calls spinning forever. */ because i cannot find any example in the git tree by grep -r mutex_unlock'.*[^a-z]'i_mutex . and the same for mutex_lock or mutex_trylock. But i see lots of comments mentioning an "i_mutex" structure element by grep -r '.*->'i_mutex . The nearest discovery to what i search for is in include/linux/fs.h static inline void inode_lock(struct inode *inode) It has lots of call instances in the code. But in contrast to "->imutex" i found not a glimpse of documentation. (Well it has enough room to hide from me ...) So is this the call by which i can reserve a cdrom device file inode before i call i_size_write() ? If so, shouldn't the comment in fs.h be "... (normally inode_lock) ..." ? Have a nice day :) Thomas ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
