Hi Guenter,
On 5 May 2015 at 00:36, Guenter Roeck linux@roeck-us.net wrote:
On Tue, May 05, 2015 at 12:03:05AM +0800, Fu Wei wrote:
Hi Guenter,
Great thanks for your feedback, will fix them ASAP. Some feedback and comment inline below :
[ ... ]
+static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);struct sbsa_gwdt_device_data *device_data =&gwdt->device_data;
u32 status;mutex_lock(&device_data->lock);status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);status &= ~SBSA_GWDT_WCS_EN;/* writing WCS will cause an explicit watchdog refresh */sbsa_gwdt_cf_write(SBSA_GWDT_WCS, status, wdd);status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);mutex_unlock(&device_data->lock);/* Check if the watchdog was disabled */if (status & SBSA_GWDT_WCS_EN)return -EACCES;Same as above.
For these two, maybe EPERM?
"Operation not permitted" seems just as wrong. Both usually reflect a software state, not a failure to write to hardware.
The question here really is if this can really happen, and, if it does, why it happens. In general it is quite uncommon for watchdog drivers to verify if such writes were successful.
I think , EIO is the best one, we config the reg to enable it , but fail, the mean we have problem on bus/IO, Do you agree?
In the given case, if it can in fact happen that the watchdog can not be stopped under some circumstances, it may make more sense to set the "nowayout" flag.
the "nowayout" means watchdog can't be stopped once it start. so I think "nowayout" is not for this case.
If that can not happen, and if you have no rational reason for assuming that the write can fail, you might just consider removing the read-back code entirely.
[ ... ]
+static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
unsigned long arg)+{
struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);void __user *argp = (void __user *)arg;int ret = -ENOIOCTLCMD;unsigned int __user *p = argp;unsigned int new_value;/* FIXME: what else do we need here?? */You'll have to be the person to answer that.
On a higher level, those ioctls need to be moved to the watchdog core, but that will have to be a separate (unrelated) patch.
Doing so :-) thanks for your suggestion !
switch (cmd) {case WDIOC_SETPRETIMEOUT:if (get_user(new_value, p))return -EFAULT;ret = sbsa_gwdt_set_pretimeout(wdd, new_value);break;case WDIOC_GETPRETIMEOUT:ret = put_user(gwdt->pretimeout, (unsigned int __user*)arg);
break;}return ret;+}
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;u32 status;unsigned int left;pr_debug("SBSA Watchdog: in %s\n", __func__);status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);if (status & SBSA_GWDT_WCS_WS0) {pr_crit("SBSA Watchdog WS0 is triggered\n");left = sbsa_gwdt_get_timeleft(wdd);pr_crit("please try to feed the dog in %ds!\n", left);Pleae no unnecessary noise.
yes, will simplify it
Really, what do you expect the user to do here ? To be online, watch the kernel log, and do something frantic ?
:-) , that is the legacy code from first version, should be delete or simplifed.
/* FIXME:anything we can do here? */
panic("SBSA Watchdog pre-timeout");Isn't this a pretimeout ?
yes, in kernel documentaion:
Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets.
So I think it makes sense to do a panic here, and if the platform
supprot
kexec/kdump, it will be trigger automatically
any suggestion?
Ok, but then you don't really need any of the additional log messages.
yes, I have deleted them.
}
return IRQ_HANDLED;+}
+static struct watchdog_info sbsa_gwdt_info = {
.identity = "SBSA Generic Watchdog",.options = WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING |WDIOF_MAGICCLOSE |WDIOF_PRETIMEOUT |WDIOF_CARDRESET,+};
+static struct watchdog_ops sbsa_gwdt_ops = {
.owner = THIS_MODULE,.start = sbsa_gwdt_start,.stop = sbsa_gwdt_stop,.ping = sbsa_gwdt_keepalive,.set_timeout = sbsa_gwdt_set_timeout,.ioctl = sbsa_gwdt_ioctl,.get_timeleft = sbsa_gwdt_get_timeleft,+};
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
struct device *dev = &pdev->dev;struct sbsa_gwdt *gwdt;struct sbsa_gwdt_device_data *device_data;struct watchdog_device *wdd;struct resource *res;void *rf_base, *cf_base;int irq;u32 status, flags;u32 w_iidr, idr;int ret = 0;/** Try to determine the frequency from the cp15 interface*/sbsa_gwdt_rate = arch_timer_get_cntfrq();/* Check the system counter frequency. */if (!sbsa_gwdt_rate)dev_err(dev, "System Counter frequency notavailable\n");
gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);if (!gwdt)return -ENOMEM;wdd = &gwdt->wdd;wdd->parent = dev;device_data = &gwdt->device_data;mutex_init(&device_data->lock);res = platform_get_resource_byname(pdev, IORESOURCE_MEM,"refresh_frame");rf_base = devm_ioremap_resource(dev, res);if (IS_ERR(rf_base))return PTR_ERR(rf_base);res = platform_get_resource_byname(pdev, IORESOURCE_MEM,"control_frame");cf_base = devm_ioremap_resource(dev, res);if (IS_ERR(rf_base))return PTR_ERR(rf_base);irq = platform_get_irq_byname(pdev, "ws0");if (IS_ERR(irq)) {dev_err(dev, "required ws0 interrupt missing\n");return PTR_ERR(irq);}res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,"ws0");
if (!res)dev_warn(dev, "ws0 interrupt flags missing,ignore\n");
elseflags = res->flags;Not really sure why you get ws0 twice. Why don't you just use platform_get_resource_byname() and use it to get the irq number as
well ?
yes, I did this before, but some suggestion said it is better to use platform_get_irq. so I am doing this way now but we can discuss more about this , and will check it again
I don't really see the flags used anywhere, so I don't really understand why you read them in the first place.
will improve it in my next patchset, we need to use flags to config gic, I am adding code. :-)
[ ... ]
+/* refresh/control frame */ +#define SBSA_GWDT_W_IIDR (0xfcc) +#define SBSA_GWDT_IDR (0xfd0)
All those ( ) are unnecessary.
for this define, I hope we can keep () to prevent someone use these uncarefully in some case,
Sorry, that doesn't make sense to me. A constant is a constant and does not need ( ) around it.
Sorry, for this , I have delete the () which are unnecessary , Thanks for your review, great help
[ ... ]
+/**
- struct sbsa_gwdt_info - SBSA Generic Watchdog device hardware
information
- @pid: GWDT ProductID
- @arch_ver: GWDT architecture version
- @revision: GWDT revision number
- @vid_bank: GWDT The JEP106 continuation code of the implementer
- @vid: GWDT The JEP106 identity code of the implementer
- @refresh_pa: the watchdog refresh frame(PA)
- @control_pa: the watchdog control frame(PA)
- */
+struct sbsa_gwdt_info {
This structure name is the same as a variable name in the driver. Please pick another name.
yes, I have found a better one for it , thanks
unsigned int pid;unsigned int arch_ver;unsigned int revision;unsigned int vid_bank;unsigned int vid;void *refresh_pa;void *control_pa;You fill out some of this data in the driver, but unless I am missing something I don't see it used, and the code says "for future use". I don't see the point of doing this. In general, if there is a plan to use such information at some later time, that is when you should introduce it.
I have deleted them, and just display them in probe function.
+};
+/**
- struct sbsa_gwdt_device_data - Internal representation of the SBSA
GWDT
- @refresh_base: VA of the watchdog refresh frame
- @control_base: VA of the watchdog control frame
- @irq: Watchdog Timer GSIV (WS0)
- @flags: Watchdog Timer Flags
- @dev: Pointer to kernel device structure
- @info: SBSA Generic Watchdog info structure
- @lock: GWDT mutex
- */
+struct sbsa_gwdt_device_data {
void __iomem *refresh_base;void __iomem *control_base;int irq;u32 flags;Is "flags" used anywhere ?
will use it
struct device *dev;Is "dev" used anywhere ?
I may delete it :-)
struct sbsa_gwdt_info info;struct mutex lock;+};
+struct sbsa_gwdt {
struct sbsa_gwdt_device_data device_data;Can you merge struct device_data into struct sbsa_gwdt ? I don't see the value of having two structures here, and it just makes the code more complicated.
yes, you are right , I have reorder those data structs, I do this way now :-) thanks
struct watchdog_device wdd;unsigned int pretimeout; /* seconds */unsigned int max_pretimeout;u64 wcv_dump;This is only set but never read. What is it for ?
I have deleted it , and just display it , if system reboot by WDT.
+#ifdef CONFIG_PM_SLEEP
u8 pm_status_store;+#endif +};
+#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd)
+#endif /* _SBSA_GENERIC_WATCHDOG_H_ */
-- Best regards,
Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021