ps3disk.c: Rewrite ps3disk_transfer #1414
Conversation
This function is bugged since the beginning, but it never hit because its variable doesn't allow. However, since commit a77e1f0 it happen now. First, it assume that ds_len will always equal to real user requested size. So it being used for sector count calculation. This is no longer true, and will fail if attempt to read last few sectors. Use bp->bio_length instead. Second, this being a loop is pointless because nsegs will never be > 1 as specified at bus_dma_tag_create() call. And all it doing is to repeat very same command again but with different ds_addr. Since bio_driver2 tag ident pointer are being reused, the result will be discarded at ps3disk_intr(). Signed-off-by: Chattrapat Sangmanee <[email protected]>
|
While I'm not sure about the contents of the patch, permissions seem to change from 644 to 755. C source files should definitely be 644. |
|
Fixed, thanks. |
| KASSERT(error == 0, | ||
| ("DMA Error %d", error)); | ||
|
|
||
| if (error) { | ||
| bp->bio_error = error; | ||
| bp->bio_flags |= BIO_ERROR; |
There was a problem hiding this comment.
Does the KASSERT() before normal error recovery makes some sense, or it is a leak of debug code?
There was a problem hiding this comment.
Latter, to me, this error should never hit at anytime.
should i remove?
There was a problem hiding this comment.
I am not sure why it should never happen. I am generally not sure how can you guarantee that data buffer will not cross a physical page boundary and so won't have more than one segment. Limiting d_maxsize to PAGE_SIZE does not guarantee that if I/O goes from user-space. May be there is some magic in bouncing buffer code of that platform to handle it, but I don't know about that. I see previous code asserts (segs[i].ds_len % sc->sc_blksize) == 0, which is a bit softer, and I guess could be partial workaround would busdma be allowed of more than one segment, but only very partial. I know nothing about this platform, and I can only hope it is not that crippled and it is just some misunderstanding.
This function is bugged since the beginning, but it never hit because its variable doesn't allow.
However, since commit a77e1f0 it happen now.
First, it assume that ds_len will always equal to real user requested size. So it being used for sector count calculation.
This is no longer true, and will fail if attempt to read last few sectors.
Use bp->bio_length instead.
Second, this being a loop is pointless because nsegs will never be > 1 as specified at bus_dma_tag_create() call.
And all it doing is to repeat very same command again but with different ds_addr. Since bio_driver2 tag ident pointer are being reused, the result will be discarded at ps3disk_intr().