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.
