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
