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=-14.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 4FC55C433E0 for ; Thu, 21 Jan 2021 21:19:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 12D8E22BEF for ; Thu, 21 Jan 2021 21:19:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 12D8E22BEF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject: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=LxlrNdeY92df3FDPNrfl/EMh/JWtctmUxvdNZrqlg4I=; b=N8CJmCRMgd+eq8T1JFuJjcjWX LUmAbRb9M7/2P8dimsqLMqt0IbTJfaEGXVo0Fa2FY0GBFI2uXGgMVXaOMT3U+SdjrprDnP/8EqXOp ZKNEyv1WWqPgStjXt76TPoxdE2rX6eSyAVsqok3NH6zH67l3xadMOdLn3sjLT8z/w5afVD1yAsDVe 9HVSEm/Xfr1JocrIIBy+lu2i1fZxblE+S4JvGw//Ce33VzPOfwLKhAtI0xKpXSKpT38wE66LilJSO uBk9N6tpzEvsdWAs760jDbptvv2m+BBvSR3xlVOHRe33ZUkw38nw/ERmgNvWjVWHzciUS281DBJn/ deetnSsYw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2hKr-0001ef-CV; Thu, 21 Jan 2021 21:17:45 +0000 Received: from mail-pf1-f178.google.com ([209.85.210.178]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2hKo-0001dq-69 for linux-arm-kernel@lists.infradead.org; Thu, 21 Jan 2021 21:17:43 +0000 Received: by mail-pf1-f178.google.com with SMTP id m6so2293382pfm.6 for ; Thu, 21 Jan 2021 13:17:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=nCPV03LmtFFgNcmlSbKeR1ygTL47bRCqEh8UZyzy3+Q=; b=AMsE05Dq0YNjjtWJKqMWAGBUHHToKG/82lDwvbg9VInIlr2s7KyayQzduW5R5o1FVr O/F+AClDld2jbHivFUp5wIsyI2+QgGAYys5AP5sPjWz++VDvr2GKxylZhxVegMBmSuAf O4iXuV5NSvsxqEfj3KwI4WxscsZbUGK8Us1eKZ00EhZAjswx5R8w5xlUezAG4awMJ/dT 6dCysFqQArfLI92xqZpUoPfiu8pN2Hwuv7Bz/lakMVhSOWKGcXgn+x9tdRHioTD7r8Sj TMhxPOneR6uZn7A/WUfDqDsnP4OWrFZrsEyFYaQIHG9YSn/TklZh4/se8RjIlImjDay2 6pTQ== X-Gm-Message-State: AOAM532GBPzSFN24Ih/uS5OvbrY2ixP2bGMth2ZDZElMCxbWKugiDUzz o5O+pd+Vw7nG27KFJ2XhOMg= X-Google-Smtp-Source: ABdhPJx4ZPV+AWlNMwICVk+gIH9l4TV9qGoMwij2g0OwOsYnUuQjDnNztMqliT7x8AdhiylRKEpAWw== X-Received: by 2002:a63:f106:: with SMTP id f6mr1227988pgi.163.1611263860562; Thu, 21 Jan 2021 13:17:40 -0800 (PST) Received: from localhost ([2601:647:5b00:1161:a4cc:eef9:fbc0:2781]) by smtp.gmail.com with ESMTPSA id l12sm6575702pjq.7.2021.01.21.13.17.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Jan 2021 13:17:39 -0800 (PST) Date: Thu, 21 Jan 2021 13:17:38 -0800 From: Moritz Fischer To: Robin Murphy Subject: Re: [PATCH] ACPI/IORT: Do not blindly trust DMA masks from firmware Message-ID: References: <20210121191612.90387-1-mdf@kernel.org> 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-20210121_161742_280953_E6D59059 X-CRM114-Status: GOOD ( 36.38 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: lorenzo.pieralisi@arm.com, sudeep.holla@arm.com, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, Moritz Fischer , moritzf@google.com, guohanjun@huawei.com, will@kernel.org, linux-arm-kernel@lists.infradead.org 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 Robin, On Thu, Jan 21, 2021 at 08:08:42PM +0000, Robin Murphy wrote: > On 2021-01-21 19:16, Moritz Fischer wrote: > > Address issue observed on real world system with suboptimal IORT table > > where DMA masks of PCI devices would get set to 0 as result. > > > > iort_dma_setup() would query the root complex' IORT entry for a DMA > > mask, and use that over the one the device has been configured with > > earlier. > > > > Ideally we want to use the minimum mask of what the IORT contains for > > the root complex and what the device was configured with, but never 0. > > > > Fixes: 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes") > > Signed-off-by: Moritz Fischer > > --- > > Hi all, > > > > not sure I'm doing this right, but I think the current behavior (while a > > corner case) seems to also fail for 32 bit devices if the IORT specifies > > 64 bit. It works on my test system now with a 32 bit device. > > I suppose it could go wrong if it's an old driver that doesn't explicitly > set its own masks and assumes they will always be 32-bit. Technically we'd > consider that the driver's fault these days, but there's a lot of legacy > around still. Huh, ok :) That's news to me. On my system I had three devices running into this, so yeah I think it's quite common. If that's the official stance I can send patches for the drivers in question :) > > > Open to suggestions for better solutions (and maybe the > > nc_dma_get_range() should have the same sanity check?) > > Honestly the more I come back to this, the more I think we should give up > trying to be clever and just leave the default masks alone beyond the > initial "is anything set up at all?" sanity checks. Setting the bus limit is > what really matters these days, and should be sufficient to encode any > genuine restriction. There's certainly no real need to widen the default > masks above 32 bits just because firmware suggests so, since the driver > should definitely be calling dma_set_mask() and friends later if it's > >32-bit capable anyway. > > > Thanks, > > Moritz > > > > --- > > drivers/acpi/arm64/iort.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index d4eac6d7e9fb..c48eabf8c121 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -1126,6 +1126,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size) > > rc = (struct acpi_iort_root_complex *)node->node_data; > > + if (!rc->memory_address_limit) { > > + dev_warn(dev, "Root complex has broken memory_address_limit\n"); > > Probably warrants a FW_BUG in there. > > > + return -EINVAL; > > + } > > + > > *size = rc->memory_address_limit >= 64 ? U64_MAX : > > 1ULL<memory_address_limit; > > @@ -1172,9 +1177,9 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > > */ > > end = dmaaddr + size - 1; > > mask = DMA_BIT_MASK(ilog2(end) + 1); > > - dev->bus_dma_limit = end; > > - dev->coherent_dma_mask = mask; > > - *dev->dma_mask = mask; > > + dev->bus_dma_limit = min_not_zero(dev->bus_dma_limit, end); > > This doesn't need to change, since the default bus limit is 0 anyway (and > that means "no limit"). Ok, I'll drop this. > > > + dev->coherent_dma_mask = min_not_zero(dev->coherent_dma_mask, mask); > > + *dev->dma_mask = min_not_zero(*dev->dma_mask, mask); I'll keep those two? > AFAICS the only way an empty mask could get here now is from > nc_dma_get_range(), so I'd rather see a consistent warning there than just > silently start working around that too. In my case the empty mask came from the pci dev branch returning a size of 1. (1 << 0). I'll replace the dev_warn() with a pr_warn(FW_BUG ...) for both {nc,rc}_dma_get_range() cases then? > > Of course IORT doesn't say these fields are optional (other than the lack of > a root complex limit in older table versions), so we're giving bad firmware > a pass to never be fixed, ho hum... I think if we yell loud enough (like FW_BUG) that'll get people's attention? Thanks for the quick reply - Moritz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel