Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9eba0937-b4cb-4c62-e339-8ff33aebdbde@quicinc.com>
Date:   Tue, 16 Aug 2022 12:22:54 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [RFC PATCH 02/14] drm/qaic: Add uapi and core driver file
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <airlied@linux.ie>, <daniel@ffwll.ch>,
        <maarten.lankhorst@linux.intel.com>, <mripard@kernel.org>,
        <tzimmermann@suse.de>
CC:     <quic_carlv@quicinc.com>, <quic_ajitpals@quicinc.com>,
        <quic_pkanojiy@quicinc.com>, <dri-devel@lists.freedesktop.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1660588956-24027-1-git-send-email-quic_jhugo@quicinc.com>
 <1660588956-24027-3-git-send-email-quic_jhugo@quicinc.com>
 <10ad6023-703d-65cf-6b00-2caa658c49e9@linaro.org>
 <e9786447-edd4-90e0-25a9-b35e96adbfc2@quicinc.com>
 <3f755d24-969b-1804-7979-880a9fe10cba@linaro.org>
From:   Jeffrey Hugo <quic_jhugo@quicinc.com>
In-Reply-To: <3f755d24-969b-1804-7979-880a9fe10cba@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/16/2022 12:00 PM, Krzysztof Kozlowski wrote:
> On 16/08/2022 20:47, Jeffrey Hugo wrote:
>>>> +static int qaic_pci_probe(struct pci_dev *pdev,
>>>> +			  const struct pci_device_id *id)
>>>> +{
>>>> +	int ret;
>>>> +	int i;
>>>> +	int mhi_irq;
>>>> +	struct qaic_device *qdev;
>>>> +
>>>> +	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
>>>> +	if (!qdev) {
>>>
>>> return -ENOMEM;
>>
>> So, no centralized exit per the coding style?  Ok.
> 
> Centralized exit except for cases where it is simply return. >
>>
>>>
>>>> +		ret = -ENOMEM;
>>>> +		goto qdev_fail;
>>>> +	}
>>>> +
>>>> +	if (id->device == PCI_DEV_AIC100) {
>>>> +		qdev->num_dbc = 16;
>>>> +		qdev->dbc = kcalloc(qdev->num_dbc, sizeof(*qdev->dbc),
>>>> +				    GFP_KERNEL);
>>>
>>> Why not devm?
>>
>> We were having issues with devm and the PCI stuff.  Looking at this
>> again, I think we can apply that here.
>>
>>>
>>>> +		if (!qdev->dbc) {
>>>> +			ret = -ENOMEM;
>>>> +			goto device_id_fail;
>>>> +		}
>>>> +	} else {
>>>> +		pci_dbg(pdev, "%s: No matching device found for device id %d\n",
>>>> +			__func__, id->device);
>>>
>>> How this can happen?
>>
>> Badly functioning hardware.  That hardware has been removed from
>> circulation.  Given that this is an init path and not performance
>> critical, having this handle the scenario in a sane way instead of
>> having the driver blow up in a weird way much later on makes me feel better.
> 
> How badly functioning hardware can bind and then report some different
> ID? If it reports different ID, it cannot bind...

It was one of those issues that was painful enough that I still remember 
it occurring, but long ago enough that I don't remember the specifics.

I don't think I'll be able to recreate the issue to re-debug it, so I'll 
just remove this.

>>>> +static int __init qaic_init(void)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (datapath_polling) {
>>>> +		poll_datapath = true;
>>>> +		pr_info("qaic: driver initializing in datapath polling mode\n");
>>>
>>> No pr() in normal path of init/exit.
>>
>> This is not the normal path.  datapath_polling is a module parameter.
>> This is something the user can enable, and so it would be good to have
>> the user informed that the option took effect.
> 
> No, not really. User always can check via sysfs and there is no point in
> polluting dmesg on module load. It might be printed (if someone has such
> modprobe setting) on every system, even without the actual device.

So, I guess this is limited to platform devices?
I see GIC, IOMMU, and PCI doing the same
I guess, will remove.

> 
>>
>>>
>>>> +	}
>>>> +
>>>> +	qaic_logging_register();
>>>> +
>>>> +	ret = mhi_driver_register(&qaic_mhi_driver);
>>>> +	if (ret) {
>>>> +		pr_debug("qaic: mhi_driver_register failed %d\n", ret);
>>>> +		goto free_class;
>>>> +	}
>>>> +
>>>> +	ret = pci_register_driver(&qaic_pci_driver);
>>>> +
>>>
>>> No need for such blank lines.
>>
>> Agreed.
>>
>>>
>>>> +	if (ret) {
>>>> +		pr_debug("qaic: pci_register_driver failed %d\n", ret);
>>>> +		goto free_mhi;
>>>> +	}
>>>> +
>>>> +	qaic_telemetry_register();
>>>> +	qaic_ras_register();
>>>> +	qaic_ssr_register();
>>>> +	goto out;
>>>
>>> return 0.
>>
>> Ok.
>>
>>>
>>>> +
>>>> +free_mhi:
>>>> +	mhi_driver_unregister(&qaic_mhi_driver);
>>>> +free_class:
>>>> +out:
>>>> +	if (ret)
>>>> +		qaic_logging_unregister();
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void __exit qaic_exit(void)
>>>> +{
>>>> +	pr_debug("qaic: exit\n");
>>>
>>> No pr() in normal path of init/exit.
>>
>> Sure.
>>
>>>
>>>> +	link_up = true;
>>>
>>> This is confusing...
>>
>> Will add a comment.  This ties into MHI, and how it can tear down in two
>> different ways, usually based on the link state.
> 
> Shouldn't this be link_up=false?

No.  module_exit() will be triggered on rmmod.  exit() will unregister 
the driver, which will cause qaic_pci_remove() to be called.  remove() 
calls qaic_mhi_free_controller() which uses the link state.

However, a hotplug event will also trigger qaic_pci_remove().

rmmod - link is up
hotplug - link is down

>> In this case, we are doing a clean tear down where the link is still up,
>> and so we should have MHI do the extra tear down that leaves the device
>> in a good state, in the event the driver gets added again.
>>
>>>
> 
> 
> 
> Best regards,
> Krzysztof