? 2016/11/23 10:29, Brian Norris ??: > On Wed, Nov 23, 2016 at 10:15:16AM +0800, Shawn Lin wrote: >> ? 2016/11/23 9:59, Brian Norris ??: >>> On Wed, Nov 23, 2016 at 09:19:11AM +0800, Shawn Lin wrote: >>>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c >>>> index b55037a..19399fc 100644 >>>> --- a/drivers/pci/host/pcie-rockchip.c >>>> +++ b/drivers/pci/host/pcie-rockchip.c > > ... > >>>> + rockchip->msg_region = ioremap(rockchip->mem_bus_addr + >>>> + ((reg_no - 1) << 20), SZ_1M); >>> >>> (And here.) >>> >>> ioremap() can fail; check for NULL. >>> >> >> Sure. >> >>> Also, when you start reusing rockchip_cfg_atu() in patch 3, you'll be >>> leaking virtual address space, as you'll keep remapping this every time. >> >> How about just check if rockchip->msg_region was already mapped? >> Otherwise we don't remap it again when calling rockchip_cfg_atu. > > That'd work, even if it's a little awkward. That's basically one of my > suggestions below. > >>> You should straighten that out. Either some kind of check for >>> 'if (!rockchip->msg_region)', or just do the map/unmap where it's >>> actually used (in patch 3). >> >> Should we really need to unmap it? As this driver won't be a module and >> I think it's okay to keep the rockchip->msg_region always. > > No, I guess we don't really need to unmap it. I just meant, you could > map/unmap every time you use it, or make sure you just map it once (and > only once). > Got it. > Also (if it helps), you could use devm_ioremap(), in case you ever do > make it removable. That sounds good. :) > > Brian > > > -- Best Regards Shawn Lin
