On 10/14/2015 02:19 PM, Peter Lieven wrote: > Am 08.10.2015 um 18:44 schrieb John Snow: >> >> On 10/08/2015 08:06 AM, Peter Lieven wrote: >>> Hi all, >>> >>> short summary from my side. The whole thing seems to get complicated, >>> let me explain why: >>> >>> 1) During review I found that the code in ide_atapi_cmd_reply_end can't >>> work correctly if the >>> byte_count_limit is not a divider or a multiple of cd_sector_size. The >>> reason is that as soon >>> as we load the next sector we start at io_buffer offset 0 overwriting >>> whatever is left in there >>> for transfer. We also reset the io_buffer_index to 0 which means if we >>> continue with the >>> elementary transfer we always transfer a whole sector (of corrupt data) >>> regardless if we >>> are allowed to transfer that much data. Before we consider fixing this I >>> wonder if it >>> is legal at all to have an unaligned byte_count_limit. It obviously has >>> never caused trouble in >>> practice so maybe its not happening in real life. >>> >> I had overlooked that part. Good catch. I do suspect that in practice >> nobody will be asking for bizarre values. >> >> There's no rule against an unaligned byte_count_limit as far as I have >> read, but suspect nobody would have a reason to use it in practice. >> >>> 2) I found that whatever cool optimization I put in to buffer multiple >>> sectors at once I end >>> up with code that breaks migration because older versions would either >>> not fill the io_buffer >>> as expected or we introduce variables that older versions do not >>> understand. This will >>> lead to problems if we migrate in the middle of a transfer. >>> >> Ech. This sounds like a bit of a problem. I'll need to think about this >> one... >> >>> 3) My current plan to get this patch to a useful state would be to use >>> my initial patch and just >>> change the code to use a sync request if we need to buffer additional >>> sectors in an elementary >>> transfer. I found that in real world operating systems the >>> byte_count_limit seems to be equal to >>> the cd_sector_size. After all its just a PIO transfer an operating >>> system will likely switch to DMA >>> as soon as the kernel ist loaded. >>> >>> Thanks, >>> Peter >>> >> It sounds like that might be "good enough" for now, and won't make >> behavior *worse* than it currently is. You can adjust the test I had >> checked in to not use a "tricky" value and we can amend support for this >> later if desired. > > Have you had a chance to look at the series with the "good enough" fix? > > Thanks, > Peter >
Will do so Friday, thanks! --js
