From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932919Ab2GLUtb (ORCPT ); Thu, 12 Jul 2012 16:49:31 -0400 Received: from mail-lb0-f174.google.com ([209.85.217.174]:61736 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756276Ab2GLUtY (ORCPT ); Thu, 12 Jul 2012 16:49:24 -0400 MIME-Version: 1.0 In-Reply-To: <4FFE3CEC.80804@huawei.com> References: <1341935655-5381-1-git-send-email-jiang.liu@huawei.com> <1341935655-5381-6-git-send-email-jiang.liu@huawei.com> <4FFCEDDE.2080907@huawei.com> <4FFD1FE7.6010504@huawei.com> <4FFE3CEC.80804@huawei.com> From: Bjorn Helgaas Date: Thu, 12 Jul 2012 14:49:02 -0600 Message-ID: Subject: Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences To: Jiang Liu Cc: Jiang Liu , Don Dutile , Yinghai Lu , Taku Izumi , "Rafael J . Wysocki" , Kenji Kaneshige , Yijing Wang , Keping Chen , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 11, 2012 at 8:56 PM, Jiang Liu wrote: > On 2012-7-12 1:52, Bjorn Helgaas wrote: >>> Hi Bjorn, >>> Seems it would be better to return error code for unimplemented >>> registers, otherwise following code will becomes more complex. A special >>> error code for unimplemented registers, such as -EIO? >> >> I think you're asking about returning error for *reads* of >> unimplemented registers? I guess I still think it's OK to completely >> hide the v1 nastiness inside these accessors, and return success with >> a zero value when reading. Having several different error returns >> seems like overkill for this case. Nobody wants to distinguish >> between different reasons for failure. >> >> I'm actually not sure that it's worth returning an error even when >> *writing* an unimplemented register. What if we return success and >> just drop the write? >> >> Maybe these should even be void functions. It feels like the only >> real use of the return value is to detect programmer error, and I >> don't think that's very effective. If we remove the return values, >> people will have to focus on the *data*, which seems more important >> anyway. > Hi Bjorn, > It's a little risk to change these PCIe capabilities access > functions as void. On some platform with hardware error detecting/correcting > capabilities, such as EEH on Power, it would be better to return > error code if hardware error happens during accessing configuration registers. > As I know, coming Intel Xeon processor may provide PCIe hardware > error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. >>> static void rtl_disable_clock_request(struct pci_dev *pdev) >>> { >>> u16 ctl; >>> >>> if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) { >>> ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN; >>> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl); >>> } >>> } >> >> I would write that as: >> >> if (!pci_is_pcie(pdev) >> return; >> >> pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl); >> if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN) >> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl & >> ~PCI_EXP_LNKCTL_CLKREQ_EN); >> >> which does the right thing regardless of what we do for return values, >> and saves a config write in the case where LNKCTL is implemented and >> CLKREQ_EN is already cleared. > When clearing a flag, we could do that. But if we are trying to set a > flag, it would be better to make sure the target register does exist. >