From: Richard Gong <[email protected]>
Add a log for user to know FPGA configuration is successful
Signed-off-by: Richard Gong <[email protected]>
---
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");
return 0;
}
--
2.7.4
Hi Richard,
On Tue, Apr 02, 2019 at 05:25:43PM -0500, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Add a log for user to know FPGA configuration is successful
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> Hi Richard,
>
> On Tue, Apr 02, 2019 at 05:25:43PM -0500, [email protected] wrote:
> > From: Richard Gong <[email protected]>
> >
> > Add a log for user to know FPGA configuration is successful
> >
> > Signed-off-by: Richard Gong <[email protected]>
> > ---
> > 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, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> Add a log for user to know FPGA configuration is successful
>>
>> Signed-off-by: Richard Gong <[email protected]>
>> ---
>> 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, [email protected] wrote:
> > > From: Richard Gong <[email protected]>
> > >
> > > Add a log for user to know FPGA configuration is successful
> > >
> > > Signed-off-by: Richard Gong <[email protected]>
> > > ---
> > > 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 <[email protected]> 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, [email protected] wrote:
> > > > From: Richard Gong <[email protected]>
> > > >
> > > > Add a log for user to know FPGA configuration is successful
> > > >
> > > > Signed-off-by: Richard Gong <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>
> On Wed, Apr 3, 2019 at 11:47 AM Moritz Fischer <[email protected]> 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, [email protected] wrote:
> > > > > From: Richard Gong <[email protected]>
> > > > >
> > > > > Add a log for user to know FPGA configuration is successful
> > > > >
> > > > > Signed-off-by: Richard Gong <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]> 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