Message-ID: <4c92160f-b2ea-c5ef-5647-6078ab47e518@intel.com>
Date:   Fri, 17 Feb 2023 12:57:24 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next 2/6] net: ipa: kill gsi->virt_raw
Content-Language: en-US
To:     Alex Elder <elder@linaro.org>
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <caleb.connolly@linaro.org>,
        <mka@chromium.org>, <evgreen@chromium.org>, <andersson@kernel.org>,
        <quic_cpratapa@quicinc.com>, <quic_avuyyuru@quicinc.com>,
        <quic_jponduru@quicinc.com>, <quic_subashab@quicinc.com>,
        <elder@kernel.org>, <netdev@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230215195352.755744-1-elder@linaro.org>
 <20230215195352.755744-3-elder@linaro.org>
 <b0b2ae77-3311-34c8-d1a2-c6f30eca3f1e@intel.com>
 <c76bbb06-b6b0-8dae-965f-95e8af3634b6@linaro.org>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <c76bbb06-b6b0-8dae-965f-95e8af3634b6@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YWVJd0grS3VnaTdsWXJ0TjNybHNBRW56bkFyVjNaL0pSbmRFZHFseHo0Y3BY?=
 =?utf-8?B?OFBhSFliaGt5U1ZJeXZiNG1LVmJrdHJhcXk1WVBYY0NudkEyRVk4TTZDQ1ZT?=
 =?utf-8?B?eC8xWkNiR2hnTlJxZUxJTFdFdVhnSkNxN0JZQks0TDhJS29VRWNqck5lU0Y1?=
 =?utf-8?B?ZjdBVkJ6TTYwQkZVVVc5VFhmQTNZKzdwanRJYXB6Z3lWS0ZUZmplNTA2cm02?=
 =?utf-8?B?c3RkUGgyUUw4ZDQxM2R3MXBxTXo0eUxCWEkrWVpaKytOYy92ZjVYcWhUTENK?=
 =?utf-8?B?Z1g0dE5lNE5ZdHVlTVJTQnlsWXU5MllkcTBUN1VBRmhDMXNqTTJOTGNLNHg3?=
 =?utf-8?B?a01OWkczdFdhVlg2NnpvWk5WY25XRUx5OUNNRVFtM3NQeVBrYXRWUDFTN2ZH?=
 =?utf-8?B?VmViZHBTRmF2WHdramx0SDAzZ1hvTkwxU241YWhPWTJtNnhOOUZkZ3VJL01C?=
 =?utf-8?B?Y0lLeG1OL0IwQU91U1JjZmRoWWhzREJ0b1hkTjRuNm9hdFFnL1ZuZEhjNUV6?=
 =?utf-8?B?THlrWUNKMGRWK25RSXB3L2IvdytRN1JHcDFaU28vemFPRXM4cFhlQ3hGOU9R?=
 =?utf-8?B?ZVN0ZkNSTGZJaUhyZHllM2FkRGNXWk9NVXp1NGtXc2hsTDFuV2Z5aVJ0VkQx?=
 =?utf-8?B?QWNrOWlpQ1JWeThLdG9rU2JkdE1ybE1tVHJZUjlKc25FS0ZlWEdQVDVUaG1Q?=
 =?utf-8?B?NDNRY0tNZFBDcXBYanpaTEdVZ2lOTzlWem40OG1sd1NEZEhFUnZ2Sm9ydEN1?=
 =?utf-8?B?cCtlUGtXeThmTTFOcDdHaktCbmpZYlRvZytMWVZnT1M3eVFlemVuRDZ2SS85?=
 =?utf-8?B?OHVFQWxQVHhsN0g5Nk1FeTJ2THZrQnpKWEFMbDErM0ZpdGdwQTFCTlJIS1dj?=
 =?utf-8?B?UGVYdU1MTlhhRHIwT3ltNEpSd1JZNUpuUm5kL0ZLR0JFZ1F3Tmk4ZlAwckc4?=
 =?utf-8?B?NThPZTQxQTVlbGxDc2hPMjhQcDJvV1ZFL2UvK1FnbEdiSFN0eXZCSStjL3RD?=
 =?utf-8?B?ZWxrYzJCQ2xLR0xuQWd1aEVCV0FYdy9xRTRyTTBFKzRoSW1YU3dDVk05TTZK?=
 =?utf-8?B?dnZZMUFHbnFPVTIycy91RHpQM2Vqd3FDMU9RN28xelRvVXVYNmpVV2FNenRq?=
 =?utf-8?B?M0Y2SWt2TDdvWWNadnBSZmtoMXZnVkpad1dkMGg0UEgzdGN1Yk05Z1k5SllO?=
 =?utf-8?B?MWdxcDFCenpZUGtjaUJkUTRGTGZnY2FwQ3JRQWo5K0ZMdG9yaFpEQnErb1Ey?=
 =?utf-8?B?cXpSblgwRnB0T2lWYU94SFhqdW1BQlJwcno1VlBCYktyM05vRXAvSWExVVZD?=
 =?utf-8?B?NWd1ZjV0WkJNVFl1ajFORm9lZ3IrZzFpQWxvdUZIVm5FSGNhQmF2RFdHbGUz?=
 =?utf-8?B?VXFtL21CUE5IeWtZaDRoM25FMktadUFUcEIzd1hhWGhCZUZoMzREd2tBbktC?=
 =?utf-8?B?OE8wc0JPYUk4WkpIRFhkOFF3dkdWcTN2UHlqUXFBQVp4UURzdnVhRmdabEo4?=
 =?utf-8?B?TVorRVB4S2ZlNzFMWEFkRFp5aTg3N3ZIVmgvc21panlqQk81NEdSMTcxZXpC?=
 =?utf-8?B?eTFvWGszVnh2NTZGLzRSQ2pPTWZSNWlyNXUxRy8yRDlycGxaZFZBR1pWenBN?=
 =?utf-8?B?VHhRUDgyYnRqSEtRclN1RDNEUU1XQkRjaXlJNGxuZlNxd041QU0vTy8rY0o1?=
 =?utf-8?B?d1ltb2JzMVdwZjJTZGpHS2ovaUpoY1Bmalo5SlNzOGxLcGtJZDBpNHRkYTJU?=
 =?utf-8?B?bmZaNUxiUmI2UzY4Q0ljV2RlWFVBTzJsL2xzSXBMTEVpS0RPNjdPcGtlRE5P?=
 =?utf-8?B?amU1SWhFc0NCODBWRzBaUWpHdTZ2cUM2OHJrRU8zZzFiVVcvbmpJakFkSTBL?=
 =?utf-8?B?SHNZK0crNFkrOW9PZm5iQzJ2L1NCVm9wbjM3NjNpcDFLQi80ei9Ia0t6MTVB?=
 =?utf-8?B?ZDQ4SVJiNlBTVWxDSTNoY2pXck40ZEkvbW1ibm13YWVoN214SExOSjFHc0tB?=
 =?utf-8?B?aXhiMUtTZThHOXhBSGRGMXNYM0E1OGFXK1VmdUN2a2IzMmo4Q3JjWVRLTmRP?=
 =?utf-8?B?MG8xNnZtMmYrNUdkTHJXQ3hMWlN3WmFEcDk5Y29wNXpaUlJybWFjcVhBWVFv?=
 =?utf-8?B?d2pGNWFtN3RCdWozTXN4aHpMaDFZOGRvMENydlZGZFNEYkRFSHQ0cnlCbW5o?=
 =?utf-8?Q?z9SuKU8l6z/WjNJ8q91unl0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 03174331-41cb-4d8a-b945-08db10de5bb5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2023 11:59:01.3758
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1JjfUORLuBPd6Ryvgz8V3wNp32nPQszueb1cq5LEgHRzUW/KikfCyJ7jC+xEI31NUrNB1DaLRgRae8/Y3cuLwdJT8B89X17bvjtgS6SPBw8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7743
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Alex Elder <elder@linaro.org>
Date: Thu, 16 Feb 2023 12:11:11 -0600

> On 2/16/23 11:51 AM, Alexander Lobakin wrote:
>> From: Alex Elder <elder@linaro.org>
>> Date: Wed, 15 Feb 2023 13:53:48 -0600

[...]

>>>       gsi->regs = gsi_regs(gsi);
>>>       if (!gsi->regs) {
>>>           dev_err(dev, "unsupported IPA version %u (?)\n",
>>> gsi->version);
>>>           return -EINVAL;
>>>       }
>>>   -    gsi->virt_raw = ioremap(res->start, size);
>>> -    if (!gsi->virt_raw) {
>>> +    gsi->virt = ioremap(res->start, size);
>>
>> Now that at least one check above went away and the second one might be
>> or be not correct (I thought ioremap core takes care of this), can't
>> just devm_platform_ioremap_resource_byname() be used here for simplicity?
> 
> Previously, virt_raw would be the "real" re-mapped pointer, and then
> virt would be adjusted downward from that.  It was a weird thing to
> do, because the result pointed to a non-mapped address.  But all uses
> of the virt pointer added an offset that was enough to put the result
> into the mapped range.
> 
> The new code updates all offsets to account for what the adjustment
> previously did.  The test that got removed isn't necessary any more.

Yeah I got it, just asked that maybe you can now use
platform_ioremap_resource_byname() instead of
platform_get_resource_byname() + ioremap() :)

> 
>>
>>> +    if (!gsi->virt) {
>>>           dev_err(dev, "unable to remap \"gsi\" memory\n");
>>>           return -ENOMEM;
>>>       }
>>> -    /* Most registers are accessed using an adjusted register range */
>>> -    gsi->virt = gsi->virt_raw - adjust;
>>>         return 0;
>>>   }
>>> @@ -170,7 +145,7 @@ int gsi_reg_init(struct gsi *gsi, struct
>>> platform_device *pdev)
>>>   /* Inverse of gsi_reg_init() */
>>>   void gsi_reg_exit(struct gsi *gsi)
>>>   {
>>> +    iounmap(gsi->virt);
>>
>> (don't forget to remove this unmap if you decide to switch to devm_)
> 
> As far as devm_*() calls, I don't use those anywhere in the driver
> currently.  If I were going to use them in one place I'd want do
> it consistently, everywhere.  I don't want to do that.

+

> 
>>>       gsi->virt = NULL;
>>> -    iounmap(gsi->virt_raw);
>>> -    gsi->virt_raw = NULL;
>>> +    gsi->regs = NULL;

[...]

>> (offtopic)
>>
>> I hope all those gsi_reg-v*.c are autogenerated? They look pretty scary
>> to be written and edited manually each time :D
> 
> I know they look scary, but no, they're manually generated and
> it's a real pain to review them.  I try to be consistent enough
> that a "diff" is revealing and helpful.  For the GSI registers,
> most of them don't change (until IPA v5.0).  I intend to modify
> this a bit further so that registers that are the same as the
> previous version don't have to be re-stated (so each new version
> only has to highlight the differences).

No, it's +/- okay to review, as you say, they're pretty consistent in
terms of code.

> 
> All that said, once created, they don't change.
> 
> Thanks.
> 
>                     -Alex
Thanks,
Olek