From: Christophe Leroy > Sent: 23 August 2022 16:26 > > Le 23/08/2022 à 17:14, Baoquan He a écrit : > > On 08/23/22 at 05:24am, Christophe Leroy wrote: > >> > >> > >> Le 23/08/2022 à 02:20, Baoquan He a écrit : > >>> On 08/22/22 at 06:25am, Christophe Leroy wrote: > >>>> > >>>> > >>>> Le 20/08/2022 à 02:31, Baoquan He a écrit : > >>>>> In some architectures, there are ARCH specifici io address mapping > >>>>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64, > >>>>> openrisc, s390, sh. > >>>>> > >>>>> In oder to convert them to take GENERIC_IOREMAP method, we need change > >>>>> the return value of hook ioremap_allowed() and iounmap_allowed(). > >>>>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect > >>>>> their current behaviour. > >>> > >>> Thanks for reviewing. > >>> > >>>> > >>>> Please don't just say you need to change the return value. Explain why. > >>> > >>> The 1st paragraph and the sentence 'In oder to convert them to take > >>> GENERIC_IOREMAP method' tell the reason, no? > >> > >> What I would like to read is _why_ you need to change the return value > >> in order to convert to GENERIC_IOREMAP > > > > I rephrase the log as below, it's OK to you? Or please help check and > > tell what I need to improve to better explain the reason. > > > > ==== > > The current io[re|un]map_allowed() hooks are used to check if the > > io[re|un]map() actions are qualified to proceed when taking > > GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map() > > will return NULL. > > > > On some architectures like arc, ia64, openris, s390, sh, there are > > ARCH specific io address mapping to translate the passed in physical > > address to io address when calling ioremap(). In order to convert > > these architectures to take GENERIC_IOREMAP way to ioremap(), we need > > change the return value of hook ioremap_allowed() and iounmap_allowed(). > > With the change, we can move the architecture specific io address > > mapping into ioremap_allowed() hook, and give the mapped io address > > out to let ioremap_prot() return it. While at it, rename the hooks to > > arch_ioremap() and arch_iounmap() to reflect their new behaviour. > > ==== > > > > That looks more in line with the type of explanation I foresee in the > commit message, thanks. I think you also need to summarise the change itself. If the success/fail return actually changes then you really need to change something so the compiler errors unchanged code. Otherwise it is a complete recipe for disaster. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
