From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E44EFC433F5 for ; Wed, 22 Sep 2021 17:57:56 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B0DAD60F4C for ; Wed, 22 Sep 2021 17:57:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B0DAD60F4C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6liB60n+ZiXAN5NA9dXBC/xbgAU9rC4rM7gbKJSrdn4=; b=v3ipVBAXshYREY 8mhBt0wJFXb1jYBJ7PcmHJvZ68HQvJaPNHPFNZBp3l7zCtFUAhsGYPBGgrr7QV7dupX+oO3rrdLG+ 1k9giz/xGysC6gjSKo+raK5FwR9SQQVt2G2u5vxoSU1KEhrvTUJvdpAnUh96SUmiYCVqyHE+DL/Sr IRkDW1hkY9IJQNALy2VqquzfkKfiRXoAywi8tozszp/LZPc4wBR85j3dAFMQ0rXnb5fCcpaZRGRMR x4hgYPjIBcigpDpn2M/iuPvjf+JX1TX8Z4Heskayl6DEOdT8qXp+vegCqYs1UA8oiAkGIn2jIr0WP DeFRRWkg/2i744NYMJOQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mT6Sw-009OTf-Ac; Wed, 22 Sep 2021 17:55:30 +0000 Received: from mail-pj1-x102a.google.com ([2607:f8b0:4864:20::102a]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mT6Ss-009OSn-6j for linux-arm-kernel@lists.infradead.org; Wed, 22 Sep 2021 17:55:27 +0000 Received: by mail-pj1-x102a.google.com with SMTP id n13-20020a17090a4e0d00b0017946980d8dso5051196pjh.5 for ; Wed, 22 Sep 2021 10:55:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=6/SvUCLfRyuYu1lKmX4nHf8QBxdiwH9kaaV9XM1Ggm8=; b=V4COAh086f0J3K5wQ255OK38XeEoSGgfq69vq27PFOFJ64c4VZxummhpm3Xv+PLJZ2 MWjIJJE8ctLSeJRv0MixAyRL+nbbFd5KlsRNxXdmD6sFrDaG5l3I7WkUyW/uk+vR3NAF uqIMKOz5iGaDnuR+/zYA2c4jXswTjp1qRk37dkbwcktYAL6Z+B21ZyWVnB6bDqMUOGEN cahlxmuYO3u4B0DuSBGtkC8TaGXEBguvWBUiJZzcuVehRIUetDQgpjgX0EmvGJOz+VO/ 4Fpb2pKHRgMXuFUsMDeOMQDWM8zQREEBmLjQA9CIYNjvJwFO3p7J/2o2Zsv/yoBeyAQu T6GA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6/SvUCLfRyuYu1lKmX4nHf8QBxdiwH9kaaV9XM1Ggm8=; b=MK7zN/QEOKAcXthos8BVHUhOwKRXQBV4lfHYh9gySZFE3OYLAtFaiv83uIZ47RqNhp l6ZY0DAd70H/+XyM5Y3Jjw3FasfTLoa8I+n52tkPaOJ3j8FfP8ymPD8uc+2H49McxJ7/ gS/xd9YC8qWEyB6dUVyM7RRZZp9fQEGIV4qy1KsIKhyjm6LumXg9gHwHQDQ/zy6QCfbp njvXng40xMXW/BGubzR+dPyOj8GvF1EiZ654GQg4vJA/fRpArl1TkwJy+/PjNNZ8ll1Y zeRB/4D8balE5/mKm5KL9yecZOFxds6dlsY+32oSVcwgJog4z5YQox6peuw2SUOtAmPZ h36g== X-Gm-Message-State: AOAM5317BI3eyAegt1wRAUT6VajUhfW5H9lumzIFXIrtkFstYFknfM+A 5Z8lIL37l9u5tVeP5QWHtScOng== X-Google-Smtp-Source: ABdhPJwrVgSn5gY/ZBiHP0dKEBG0qvC4QE9UF3HQyT382gRtLxnLi3BKane1/nUm25ZgdhIIk4atBA== X-Received: by 2002:a17:90b:3b41:: with SMTP id ot1mr12765719pjb.186.1632333324544; Wed, 22 Sep 2021 10:55:24 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id n66sm3132665pfn.142.2021.09.22.10.55.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Sep 2021 10:55:23 -0700 (PDT) Date: Wed, 22 Sep 2021 11:55:21 -0600 From: Mathieu Poirier To: Shengjiu Wang Cc: Shengjiu Wang , Ohad Ben Cohen , Bjorn Andersson , Rob Herring , Shawn Guo , Sascha Hauer , Sascha Hauer , Fabio Estevam , Daniel Baluta , NXP Linux Team , "open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" , linux-kernel Subject: Re: [PATCH v4 3/4] remoteproc: imx_dsp_rproc: Add remoteproc driver for DSP on i.MX Message-ID: <20210922175521.GA2157824@p14s> References: <1631092255-25150-1-git-send-email-shengjiu.wang@nxp.com> <1631092255-25150-4-git-send-email-shengjiu.wang@nxp.com> <20210915161624.GA1770838@p14s> <20210916165957.GA1825273@p14s> <20210917152236.GA1878943@p14s> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210922_105526_328295_F7013F4D X-CRM114-Status: GOOD ( 48.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 22, 2021 at 09:35:54AM +0800, Shengjiu Wang wrote: > Hi Mathieu > > On Fri, Sep 17, 2021 at 11:22 PM Mathieu Poirier > wrote: > > > > On Fri, Sep 17, 2021 at 05:44:44PM +0800, Shengjiu Wang wrote: > > > On Fri, Sep 17, 2021 at 1:20 PM Shengjiu Wang wrote: > > > > > > > > On Fri, Sep 17, 2021 at 1:00 AM Mathieu Poirier > > > > wrote: > > > > > > > > > > [...] > > > > > > > > > > > > > + > > > > > > > > +/** > > > > > > > > + * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory > > > > > > > > + * @rproc: remote processor which will be booted using these fw segments > > > > > > > > + * @fw: the ELF firmware image > > > > > > > > + * > > > > > > > > + * This function specially checks if memsz is zero or not, otherwise it > > > > > > > > + * is mostly same as rproc_elf_load_segments(). > > > > > > > > + */ > > > > > > > > +static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc, > > > > > > > > + const struct firmware *fw) > > > > > > > > +{ > > > > > > > > + struct device *dev = &rproc->dev; > > > > > > > > + u8 class = fw_elf_get_class(fw); > > > > > > > > + u32 elf_phdr_get_size = elf_size_of_phdr(class); > > > > > > > > + const u8 *elf_data = fw->data; > > > > > > > > + const void *ehdr, *phdr; > > > > > > > > + int i, ret = 0; > > > > > > > > + u16 phnum; > > > > > > > > + > > > > > > > > + ehdr = elf_data; > > > > > > > > + phnum = elf_hdr_get_e_phnum(class, ehdr); > > > > > > > > + phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr); > > > > > > > > + > > > > > > > > + /* go through the available ELF segments */ > > > > > > > > + for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) { > > > > > > > > + u64 da = elf_phdr_get_p_paddr(class, phdr); > > > > > > > > + u64 memsz = elf_phdr_get_p_memsz(class, phdr); > > > > > > > > + u64 filesz = elf_phdr_get_p_filesz(class, phdr); > > > > > > > > + u64 offset = elf_phdr_get_p_offset(class, phdr); > > > > > > > > + u32 type = elf_phdr_get_p_type(class, phdr); > > > > > > > > + void *ptr; > > > > > > > > + bool is_iomem; > > > > > > > > + > > > > > > > > + if (type != PT_LOAD || !memsz) > > > > > > > > > > > > > > You did a really good job with adding comments but this part is undocumented... > > > > > > > If I read this correctly you need to check for !memsz because some part of > > > > > > > the program segment may have a header but its memsz is zero, in which case it can > > > > > > > be safely skipped. So why is that segment in the image to start with, and why > > > > > > > is it marked PT_LOAD if it is not needed? This is very puzzling... > > > > > > > > > > > > Actually I have added comments in the header of this function. > > > > > > > > > > Indeed there is a mention of memsz in the function's header but it doesn't > > > > > mention _why_ this is needed, and that is what I'm looking for. > > > > > > > > > > > > > > > > > memsz= 0 with PT_LOAD issue, I have asked the toolchain's vendor, > > > > > > they said that this case is allowed by elf spec... > > > > > > > > > > > > And in the "pru_rproc.c" and "mtk_scp.c", seems they met same problem > > > > > > they also check the filesz in their internal xxx_elf_load_segments() function. > > > > > > > > > > In both cases they are skipping PT_LOAD sections where "filesz" is '0', which > > > > > makes sense because we don't know how many bytes to copy. But here you are > > > > > skipping over a PT_LOAD section with a potentially valid filesz, and that is the > > > > > part I don't understand. > > > > > > > > Ok, I can use filesz instead. For my case, filesz = memsz = 0, > > > > it is the same result I want. > > > > If that is the case then rproc_elf_load_segments() should work, i.e it won't > > copy anything. If rproc_elf_load_segments() doesn't work for you then there are > > corner cases you haven't told me about. > > > > > > > > > > The reason why I use "memsz '' is because there is "if (filesz > memsz) " > > > > check after this, if memsz is zero, then "filesz" should be zero too, other > > > > values are not allowed. > > > > > > But I still think checking "!memsz" is better than filesz, because > > > memsz > filesz is allowed (filesz = 0), the code below can be executed. > > > filesz > memsz is not allowed. The question remains the same - have you seen instances where memsz > filesz? Also, can you point me to the reference where it is said that memsz is allowed? And if it is allowed than how do we know that this program section has valid data, because after all, filesz is 0? > > > > > > What do you think? > > > > I don't see a need to add a custom implementation for things that _may_ happen. > > If using the default rproc_elf_load_segments() works than go with that. We can deal > > with problems if/when there is a need for it. > > > > The default rproc_elf_load_segments() with filesz = memsz = 0, then the > rproc_da_to_va() return ptr=NULL, then rproc_elf_load_segments() will return > with error. So this is the reason to add a custom implementation. Ok, I see about rproc_da_to_va() returning NULL and failing everything from there one. > > best regards > wang shengjiu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel