| Message ID | 1554243943-25507-1-git-send-email-richard.gong@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [PATCHv1] fpga: mgr: add FPGA configuration log | expand |
Hi Richard, On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote: > From: Richard Gong <richard.gong@intel.com> > > Add a log for user to know FPGA configuration is successful > > Signed-off-by: Richard Gong <richard.gong@intel.com> > --- > drivers/fpga/fpga-mgr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index c386681..559e046 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, > } > mgr->state = FPGA_MGR_STATE_OPERATING; > > + dev_info(&mgr->dev, "Successfully programming FPGA\n"); That info is available in FPGA manager's sysfs status entry, if at all I'd make this a dev_dbg(). From my end I don't see how we need this really. Thanks, Moritz
On Wed, Apr 3, 2019 at 9:20 AM Moritz Fischer <mdf@kernel.org> wrote: > > Hi Richard, > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote: > > From: Richard Gong <richard.gong@intel.com> > > > > Add a log for user to know FPGA configuration is successful > > > > Signed-off-by: Richard Gong <richard.gong@intel.com> > > --- > > drivers/fpga/fpga-mgr.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > index c386681..559e046 100644 > > --- a/drivers/fpga/fpga-mgr.c > > +++ b/drivers/fpga/fpga-mgr.c > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, > > } > > mgr->state = FPGA_MGR_STATE_OPERATING; > > > > + dev_info(&mgr->dev, "Successfully programming FPGA\n"); > > That info is available in FPGA manager's sysfs status entry, if at all > I'd make this a dev_dbg(). > > From my end I don't see how we need this really. I'm ok with adding a message. It's not adding lots of messages, just one line for an event that will only happen for people who care about the event (not too many FPGA users but if someone is using FPGA, they will care about this.) Alan > > Thanks, > Moritz
Hi Moritz, On 4/3/19 9:20 AM, Moritz Fischer wrote: > Hi Richard, > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote: >> From: Richard Gong <richard.gong@intel.com> >> >> Add a log for user to know FPGA configuration is successful >> >> Signed-off-by: Richard Gong <richard.gong@intel.com> >> --- >> drivers/fpga/fpga-mgr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c >> index c386681..559e046 100644 >> --- a/drivers/fpga/fpga-mgr.c >> +++ b/drivers/fpga/fpga-mgr.c >> @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, >> } >> mgr->state = FPGA_MGR_STATE_OPERATING; >> >> + dev_info(&mgr->dev, "Successfully programming FPGA\n"); > > That info is available in FPGA manager's sysfs status entry, if at all > I'd make this a dev_dbg(). > > From my end I don't see how we need this really. We got requests from the field and they want to see a log to get know if FPGA configuration is successfully completed. They don't want use any additional command to get status. This log is useful for the user who performs FPGA configuration. I think we need use dev_info, since dev_dbg is not enabled by fault for most build. > > Thanks, > Moritz >
Hi Richard, On Wed, Apr 03, 2019 at 11:43:26AM -0500, Richard Gong wrote: > Hi Moritz, > > > On 4/3/19 9:20 AM, Moritz Fischer wrote: > > Hi Richard, > > > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote: > > > From: Richard Gong <richard.gong@intel.com> > > > > > > Add a log for user to know FPGA configuration is successful > > > > > > Signed-off-by: Richard Gong <richard.gong@intel.com> > > > --- > > > drivers/fpga/fpga-mgr.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > > index c386681..559e046 100644 > > > --- a/drivers/fpga/fpga-mgr.c > > > +++ b/drivers/fpga/fpga-mgr.c > > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, > > > } > > > mgr->state = FPGA_MGR_STATE_OPERATING; > > > + dev_info(&mgr->dev, "Successfully programming FPGA\n"); > > > > That info is available in FPGA manager's sysfs status entry, if at all > > I'd make this a dev_dbg(). > > > > From my end I don't see how we need this really. > > We got requests from the field and they want to see a log to get know if > FPGA configuration is successfully completed. They don't want use any > additional command to get status. > > This log is useful for the user who performs FPGA configuration. > > I think we need use dev_info, since dev_dbg is not enabled by fault for most > build. Well basically it boils down to: $ dmesg | grep "Sucessfully" vs $ cat /sys/class/fpga.../status Personally not in favor of extra messages, but if we do it we should change the message to "Sucessfully programmed FPGA". I think making it a dbg message is a good trade-off ... Moritz
On Wed, Apr 3, 2019 at 11:47 AM Moritz Fischer <mdf@kernel.org> wrote: > > Hi Richard, > > On Wed, Apr 03, 2019 at 11:43:26AM -0500, Richard Gong wrote: > > Hi Moritz, > > > > > > On 4/3/19 9:20 AM, Moritz Fischer wrote: > > > Hi Richard, > > > > > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote: > > > > From: Richard Gong <richard.gong@intel.com> > > > > > > > > Add a log for user to know FPGA configuration is successful > > > > > > > > Signed-off-by: Richard Gong <richard.gong@intel.com> > > > > --- > > > > drivers/fpga/fpga-mgr.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > > > index c386681..559e046 100644 > > > > --- a/drivers/fpga/fpga-mgr.c > > > > +++ b/drivers/fpga/fpga-mgr.c > > > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, > > > > } > > > > mgr->state = FPGA_MGR_STATE_OPERATING; > > > > + dev_info(&mgr->dev, "Successfully programming FPGA\n"); > > > > > > That info is available in FPGA manager's sysfs status entry, if at all > > > I'd make this a dev_dbg(). > > > > > > From my end I don't see how we need this really. > > > > We got requests from the field and they want to see a log to get know if > > FPGA configuration is successfully completed. They don't want use any > > additional command to get status. > > > > This log is useful for the user who performs FPGA configuration. > > > > I think we need use dev_info, since dev_dbg is not enabled by fault for most > > build. > > Well basically it boils down to: > > $ dmesg | grep "Sucessfully" > > vs > > $ cat /sys/class/fpga.../status it's state, not status for most fpga manager drivers. It should return 'operating' if everything went well. It seems like there's a possible scenario where the FPGA starts up with the FPGA in 'operating' mode and the user messes up early enough that the state doesn't change. > > Personally not in favor of extra messages, but if we do it we should > change the message to "Sucessfully programmed FPGA". > > I think making it a dbg message is a good trade-off ... > > Moritz
On Wed, Apr 3, 2019 at 1:05 PM Alan Tull <atull@kernel.org> wrote: > > On Wed, Apr 3, 2019 at 11:47 AM Moritz Fischer <mdf@kernel.org> wrote: > > > > Hi Richard, > > > > On Wed, Apr 03, 2019 at 11:43:26AM -0500, Richard Gong wrote: > > > Hi Moritz, > > > > > > > > > On 4/3/19 9:20 AM, Moritz Fischer wrote: > > > > Hi Richard, > > > > > > > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote: > > > > > From: Richard Gong <richard.gong@intel.com> > > > > > > > > > > Add a log for user to know FPGA configuration is successful > > > > > > > > > > Signed-off-by: Richard Gong <richard.gong@intel.com> > > > > > --- > > > > > drivers/fpga/fpga-mgr.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > > > > > index c386681..559e046 100644 > > > > > --- a/drivers/fpga/fpga-mgr.c > > > > > +++ b/drivers/fpga/fpga-mgr.c > > > > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, > > > > > } > > > > > mgr->state = FPGA_MGR_STATE_OPERATING; > > > > > + dev_info(&mgr->dev, "Successfully programming FPGA\n"); > > > > > > > > That info is available in FPGA manager's sysfs status entry, if at all > > > > I'd make this a dev_dbg(). > > > > > > > > From my end I don't see how we need this really. > > > > > > We got requests from the field and they want to see a log to get know if > > > FPGA configuration is successfully completed. They don't want use any > > > additional command to get status. > > > > > > This log is useful for the user who performs FPGA configuration. > > > > > > I think we need use dev_info, since dev_dbg is not enabled by fault for most > > > build. > > > > Well basically it boils down to: > > > > $ dmesg | grep "Sucessfully" > > > > vs > > > > $ cat /sys/class/fpga.../status > > it's state, not status for most fpga manager drivers. It should > return 'operating' if everything went well. > > It seems like there's a possible scenario where the FPGA starts up > with the FPGA in 'operating' mode and the user messes up early enough > that the state doesn't change. > > > > > Personally not in favor of extra messages, but if we do it we should > > change the message to "Sucessfully programmed FPGA". > > > > I think making it a dbg message is a good trade-off ... dbg vs info... On the one hand, it is a usually a message the developer wants to see so the developer would turn on debug messages. But then again FPGA programming doesn't happen that often and it is a kind of significant event since it is your hardware changing i.e. it won't add a lot messages, but it is sort of an important one if it happens. If the system crashes after a FPGA reprogramming event, it would be good to have this in the log by default. I don't want to argue too powerfully for adding extra messages though. Is this a case where info is worth it since fpga programming is significant? > > > > Moritz
On Wed, Apr 03, 2019 at 01:37:51PM -0500, Alan Tull wrote: > > > > it's state, not status for most fpga manager drivers. It should > > return 'operating' if everything went well. Yeah, sorry :) > > It seems like there's a possible scenario where the FPGA starts up > > with the FPGA in 'operating' mode and the user messes up early enough > > that the state doesn't change. Huh, then we should fix that instead? :) > > > > > > > > Personally not in favor of extra messages, but if we do it we should > > > change the message to "Sucessfully programmed FPGA". > > > > > > I think making it a dbg message is a good trade-off ... > > dbg vs info... On the one hand, it is a usually a message the > developer wants to see so the developer would turn on debug messages. > But then again FPGA programming doesn't happen that often and it is a > kind of significant event since it is your hardware changing i.e. it > won't add a lot messages, but it is sort of an important one if it > happens. If the system crashes after a FPGA reprogramming event, it > would be good to have this in the log by default. I don't want to > argue too powerfully for adding extra messages though. Is this a case > where info is worth it since fpga programming is significant? In the current setup, it doesn't happen often. Going forward people might have use-cases where this happens a lot more often. I mean if y'all feel like this is required, sure, I still feel people shouldn't rely on dmesg output for functional verification :) I don't wanna guarantee that this message is gonna be there always ... Cheers, Moritz
On Wed, Apr 3, 2019 at 3:08 PM Moritz Fischer <mdf@kernel.org> wrote: > > On Wed, Apr 03, 2019 at 01:37:51PM -0500, Alan Tull wrote: > > > > > > it's state, not status for most fpga manager drivers. It should > > > return 'operating' if everything went well. > > Yeah, sorry :) > > > > It seems like there's a possible scenario where the FPGA starts up > > > with the FPGA in 'operating' mode and the user messes up early enough > > > that the state doesn't change. > > Huh, then we should fix that instead? :) > > > > > > > > > > > Personally not in favor of extra messages, but if we do it we should > > > > change the message to "Sucessfully programmed FPGA". > > > > > > > > I think making it a dbg message is a good trade-off ... > > > > dbg vs info... On the one hand, it is a usually a message the > > developer wants to see so the developer would turn on debug messages. > > But then again FPGA programming doesn't happen that often and it is a > > kind of significant event since it is your hardware changing i.e. it > > won't add a lot messages, but it is sort of an important one if it > > happens. If the system crashes after a FPGA reprogramming event, it > > would be good to have this in the log by default. I don't want to > > argue too powerfully for adding extra messages though. Is this a case > > where info is worth it since fpga programming is significant? > > In the current setup, it doesn't happen often. Going forward people > might have use-cases where this happens a lot more often. Yes, then adding the message could become very spammy. > > I mean if y'all feel like this is required, sure, I still feel people > shouldn't rely on dmesg output for functional verification :) I agree with you that using sysfs to see what state the FPGA mgr ended up in should be adequate most of the time. We probably don't need this. > > I don't wanna guarantee that this message is gonna be there always ... Indeed... Thanks, Alan > > Cheers, > Moritz
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index c386681..559e046 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr, } mgr->state = FPGA_MGR_STATE_OPERATING; + dev_info(&mgr->dev, "Successfully programming FPGA\n"); return 0; }