On 18 March 2015 at 10:18, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: >> >> This changelog still sucks. It doesn't describe the effect of this >> behavior change for the user. It doesn't even make it clear that you >> are aware that this is a behavior change. > > It doesn't say to me that you have asked yourself if the sparse > annotations are correct. Many times they are wrong. My understanding, which as a new contributor is of course limited and likely simply wrong in many aspects, is - these memset's are referring to I/O mapped memory, which as far as I can tell is actually the case here, so in order to make it explicit that this is the case and we know it is, we use memset_io. As far as I understand the pointers simply have a modifier applied which marks them as I/O mapped memory for the purposes of sparse checking whether they are used consistently as such and are not treated like they are a normal kernel pointer. In this case the cursor->vstart and crtc->vScreen pointers, looking through the source, explicitly refer to memory which is I/O mapped, and is annotated as __iomem accordingly throughout. I will update the message accordingly, obviously if I'm misunderstanding something let me know. > We have had this discussion before but you still sent the same exact > bad changelog. Actually you said:- > When I see a patch like this, then I worry, "What if the Sparse > annotations are wrong? The patch description doesn't say anything about > that." After review then I think the annotations are correct so that's > fine. And:- > Yes. The patch is correct. I wasn't asking you to redo it. From later > patches it's actually clear that you know that this change is a bugfix > and a behavior change. But we get a lot of patches where people just > randomly change things to please Sparse and it maybe silences a warning > but it's not correct. I can think of a few recentish examples where > people used standard struct types which hold __iomem or __user pointers > but they used them in non-standard ways so the pointers were actually > normal kernel pointers. So it wasn't clear *to me* you wanted me to change that, given you asked me *not* to redo it explicitly (which I assumed applied to the message too) - apologies if I misinterpreted this! Best, -- Lorenzo Stoakes https:/ljs.io _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
