Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp2144882yba; Mon, 15 Apr 2019 06:01:15 -0700 (PDT) X-Google-Smtp-Source: APXvYqxH9YQqBIK6mf+vZlTT8N8bxCmBQNJLPKqOpnODXafecx1J9uL8TW+1M8FRFQQiDtxc5p4Q X-Received: by 2002:a62:b418:: with SMTP id h24mr74903412pfn.145.1555333275633; Mon, 15 Apr 2019 06:01:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1555333275; cv=none; d=google.com; s=arc-20160816; b=KK5a8kH+1udRNlZ3gbiksT/SsTfeu98inGgXCWTU45Ck5b3Yg6d8VrguZ+96L3OqPz MQ7Ng2iU9N6V2WSRQ6YRC4X2sFVA3WFGs8LniP0a7WNn1cHTtjFp1lXr8nhCO84jf9E8 UtS5vNwlaXgqvQ8sIcEfsThON8cU9M8L6FNBThwfZsk/gsSVZwXZ9+XzFzzg+R2bNmDQ mri3GN6A0qc4f2MoyzGFTIl+O1ZT+rbRIT6cA3Ury0wAFV1F4taIVpm+fiOXIJNtJVvi fEYqj8NLjO3Fgr+xh2CGWvBxC3nfObBMh+3odpRCEZFTHBa+rEpatCUXLStoPy9HjSsd XGQw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=aUX0TNiGq/KgiwnRmF3oEw42hvyn4Vb6l5AVU+7itU4=; b=a76ZPyXVe3ZHLSXNy1MdSf+t+JmekHnphF7VKZj+L8KHOLBSmNZnFlgO/cno9CdDyS 5V8np/BvPZABYcthEBImAAMQOiS6X85g82rx3jPn03Xaw9S2ejbFHydEgRupiOJ2L+e8 gjsdyviPtoMQmKqV/0JMhpCMgSdFEzT/V6uwzs+CLSQ9uzErDYDdEFuD/svCdXJKGoE/ WNP5vNz55LcWtHldlkbtSJozgLGbNI96Vlond3tB4XFp1sUE3JCPXrNN0KyYpGCPuWJX BH3ILX+iALYZOhqubMLEyNL6fJjsItFMpveAQPaq+xaHORx9d/wyef0NfoB4KAvvViM6 2jgw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id q13si45448850pfh.162.2019.04.15.06.00.58; Mon, 15 Apr 2019 06:01:15 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727699AbfDOM7u (ORCPT + 99 others); Mon, 15 Apr 2019 08:59:50 -0400 Received: from mail-lj1-f195.google.com ([209.85.208.195]:41383 "EHLO mail-lj1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727515AbfDOM7t (ORCPT ); Mon, 15 Apr 2019 08:59:49 -0400 Received: by mail-lj1-f195.google.com with SMTP id k8so15542309lja.8 for ; Mon, 15 Apr 2019 05:59:47 -0700 (PDT) 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:user-agent; bh=aUX0TNiGq/KgiwnRmF3oEw42hvyn4Vb6l5AVU+7itU4=; b=kNilKxSdKz8ExW8iXqKrp6QdHeMekO4Nwu63twoadCaNvlw5SwkZ6cfoXEhBhPsT00 YwHZRzymxZbYL6xXqH4yLlomzkwNN7Zc/I1PdMfLOXV/B8S8eGZ3/vf7goK2GETEj58L ih4in7DObmFjqQk1HjDAdRbnsqHfpd55Sag26dNg1LUCFNgqm9spor+wdZRT822/7Kp1 tRMmyD8KoFJBALYxdkawxjrH3Yv+LgTii+JGLbbIETp/+uHy9ZLYAxcUKJAaaP96mFP4 YAPAdYWEN82Gf2cR8baaCx/z6rhAo/GpMz2YhQA9Vpc0BITN536WYSC60a3F5SnjMpN9 LGXg== X-Gm-Message-State: APjAAAWV+nZui75aLNwYanLpvK0me1QsRtbLFMzIWeS/jMj7iZb3DcYe MfJzMPKgEaLEv2PvXwro9Hs= X-Received: by 2002:a2e:7806:: with SMTP id t6mr11739988ljc.143.1555333185949; Mon, 15 Apr 2019 05:59:45 -0700 (PDT) Received: from xi.terra (c-74bee655.07-184-6d6c6d4.bbcust.telenor.se. [85.230.190.116]) by smtp.gmail.com with ESMTPSA id 73sm10674759ljf.72.2019.04.15.05.59.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 15 Apr 2019 05:59:44 -0700 (PDT) Received: from johan by xi.terra with local (Exim 4.91) (envelope-from ) id 1hG1DB-0000ek-VA; Mon, 15 Apr 2019 14:59:50 +0200 Date: Mon, 15 Apr 2019 14:59:49 +0200 From: Johan Hovold To: Alex Elder Cc: Dan Carpenter , Madhumitha Prabakaran , devel@driverdev.osuosl.org, elder@kernel.org, johan@kernel.org, linux-kernel@vger.kernel.org, greybus-dev@lists.linaro.org Subject: Re: [greybus-dev] [PATCH] Staging: greybus: Fix spinlock_t definition without comment Message-ID: <20190415125949.GB775@localhost> References: <20190405200046.20600-1-madhumithabiw@gmail.com> <20190405205304.GS32590@kadam> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.4 (2019-03-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 05, 2019 at 05:50:10PM -0500, Alex Elder wrote: > On 4/5/19 3:53 PM, Dan Carpenter wrote: > > On Fri, Apr 05, 2019 at 03:00:46PM -0500, Madhumitha Prabakaran > > wrote: > >> Fix spinlock_t definition without comment. > >> > >> Signed-off-by: Madhumitha Prabakaran > > Madhumitha, the reason one would want a comment associated with > a lock field in a structure is to get some understanding of why > it's needed. Saying "protect structure fields" is not enough, > because that can pretty much be assumed, so a comment like that > adds no value. > > Looking at the code, you can see the lock field protects the > connection's operations list. It also appears to be needed > for accessing the state field (reading or updating). > > Given that, a better comment might be: > > spinlock_t lock; /* operations list and state */ > > >> --- drivers/staging/greybus/connection.h | 2 +- 1 file changed, 1 > >> insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/greybus/connection.h > >> b/drivers/staging/greybus/connection.h index > >> 5ca3befc0636..0aedd246e94a 100644 --- > >> a/drivers/staging/greybus/connection.h +++ > >> b/drivers/staging/greybus/connection.h @@ -47,7 +47,7 @@ struct > >> gb_connection { unsigned long flags; > >> > >> struct mutex mutex; - spinlock_t lock; + spinlock_t lock; /* > >> Protect structure fields */ enum gb_connection_state state; > > > > What does the mutex do then? Why can't we just use the spinlock for > > everything? > > The mutex needs to be held during enable and disable of a connection. > Johan might be able to give you a more complete answer but these > operations (or at least the enable) need to block, so can't hold a > spinlock. Yeah, I should have documented this at the time. You're right that the connection spinlock protects the operation list, and the mutex the connection state, but there are some other dependencies here and I don't have time to look at this at the moment. Better to leave as is, I'd say. Johan